Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dexed:setName/getName weirdness #501

Open
diyelectromusic opened this issue May 27, 2023 · 1 comment
Open

Dexed:setName/getName weirdness #501

diyelectromusic opened this issue May 27, 2023 · 1 comment

Comments

@diyelectromusic
Copy link
Contributor

In MiniDexed.cpp the functions GetVoiceName (https://github.com/probonopd/MiniDexed/blob/main/src/minidexed.cpp#L930) and SetVoiceName (https://github.com/probonopd/MiniDexed/blob/main/src/minidexed.cpp#L1566) use Dexed::setName/getName (https://codeberg.org/dcoredump/Synth_Dexed/src/branch/master/src/dexed.cpp#L1699), but I'm wondering if the sense of these functions in the Dexed library might be incorrect?

GetVoiceName uses setName to read the name from Dexed, and SetVoiceName uses getName to write it.

In Dexed the code is:

void Dexed::setName(char* name)
{
  strncpy(name, (char*)&data[DEXED_VOICE_OFFSET + DEXED_NAME], 10);
 }
  
void Dexed::getName(char* buffer)
{
   strncpy((char*)&data[DEXED_VOICE_OFFSET + DEXED_NAME], buffer, 10);
   buffer[10] = '\0';
}

So as strncpy takes parameters in the order (dest, src, len) these do seem to be doing the opposite of what their names imply, which means our code is correct.

However, there are some oddities:

  • getName sets the last character of the provided buffer to \0, which implies it was meant to be filling in the buffer not reading from it.
  • setName does nothing with the provided buffer but the VOICE DATA name is not a NULL terminated string, it is simply 10 characters.
  • In SetVoiceName a buffer of only 10 characters is provided to getName, which implies that the final "buffer[10]='\0'" will cause a buffer overrun...
  • In GetVoiceName we use a buffer of 11 characters and preset them all to 0 which means the non-null terminated string is ok.

So in short, I think the parameters to the strncpy calls in get/setName are the wrong way round, that we have a bug in our SetVoiceName and that our functions ought to be properly paired with the get/set in Dexed....

Thoughts?
Kevin

@BobanSpasic
Copy link

BobanSpasic commented May 28, 2023

I do not know the data structures in Dexed, but it does not seem wrong to me.
SetName puts the string 'name' into the DEXED_VOICE_OFFSET structure at the position DEXED_NAME, and just the 10 chars, ignoring the rest if longer.
GetName gets 10 chars from DEXED_VOICE_OFFSET and put them into 'buffer' (initialized somewhere else, before the function call), again just in length from 10 chars. Add a null char (string terminator) to make it a fully qualified string.

EDIT: I see - it looks like the source and the destination are in the wrong positions in stpncpy call. Did you check the standard C library used in MiniDexed?

EDIT 2: Checked, it is the same in circle newlib just like in any other C lib:
stpncpy (char *__restrict dst, const char *__restrict src, size_t count)

https://github.com/smuehlst/circle-newlib/blob/6fe4bb7350b47e199093f5460226dec53506ccaf/newlib/libc/string/stpncpy.c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants