Dexed:setName/getName weirdness

[diyelectromusic]

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]

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

[probonopd]

@diyelectromusic do you think this is still an open issue? Thanks.

[diyelectromusic]

I'm not aware of anything changing with respect to this, as far as I can see, the set function in MiniDexed still has to call get in Synth_Dexed and vice versa.

Kevin

[soyersoyer]

It's fixed in a newer version of Synth_Dexed https://codeberg.org/dcoredump/Synth_Dexed/commit/c97597f90024326a1b29d02888fc44c92f248547 And there is a commit for MiniDexed also: https://github.com/probonopd/MiniDexed/commit/f2246347ad9da78553e1fcb9c75d5412699396fb

[probonopd]

Hi @soyersoyer, "This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository." Would you like to open a pull request for it? Thanks.

Do we need to update both Synth_Dexed and apply your patch at the same time for this to work?

[soyersoyer]

Yes. https://github.com/probonopd/MiniDexed/pull/871

[probonopd]

Merged, so closing. Please comment here in case the issue persists. Thanks!