My variable is getting corrupted

Status
Not open for further replies.

sw_hunt

Well-known member
Hi, I set int synthType to 2, print it (OK), call routine(), print it again (not OK, blank space in the below example, though I've seen 0 or long numbers instead depending on whether other code precedes it).

If I change things like replace numOfBanksPerSynth in routine() with 4 then it's OK again. If I increase the 3 in char dirName[3... then it's OK. If I paste the contents of routine() in setup, ie do the same code but without a call, then it's ok again! Please what am I doing wrong?

Thanks


HTML:
const char *listOfSynthTypes[] = {"OB6", "Prophet6", "CCsynth", "Prologue", "Wave2"};
int synthType = 0;
int numOfBanksPerSynth = 4;

void setup() {
  Serial.begin(9600);//diagnostic serial port
  synthType = 2; 
  Serial.print("synthType is ");Serial.println(synthType);
  routine();
  Serial.print("synthType is now ");Serial.println(synthType);
 
}  // end setup

void loop() {
}

void routine() {
  for (int i = 0; i < 5; i++) {
    for (int j = 1; j < numOfBanksPerSynth + 1; j++) {
      char dirName[3 + strlen(listOfSynthTypes[i])];
      sprintf(dirName, "%s/%.2d", listOfSynthTypes[i], j);
    }
  }
}
 
I don't see int he code you pasted where for example dirName is defined, so sometimes hard to know, what is going on. Maybe dirName is not long enough to hold the whole thing and memory get corrupted.

In cases like this I prefer not using calls like sprintf... If you wish to use it, then use snprintf... like:
Code:
 snprintf(dirName, sizeof(dirName), "%s/%.2d", listOfSynthTypes[i], j);
But again hard to know from the information posted.
 
strlen is not including the null character when it gives you the length. You should be using:
Code:
char dirName[4 + strlen(listOfSynthTypes[i])];

Instead of:
Code:
char dirName[3 + strlen(listOfSynthTypes[i])];

...notice the 4 instead of the 3. So, sprintf is overrunning the dirName buffer that you're allocating and corrupting your synthType variable.
 
dirName is defined in the middle of routine(). It's only used inside routine(). This is an excerpt from a much bigger sketch where I actually use dirName to create some SD directories but again all from inside routine().

The full version of routine() is called checkAndCreateSynthDirectories

Code:
void checkAndCreateSynthDirectories () {

  for (int i = 0; i < numOflistOfSynthTypes; i++) {
    for (int j = 1; j < numOfBanksPerSynth + 1; j++) {
      char dirName[3 + strlen(listOfSynthTypes[i])];
       sprintf(dirName, "%s/%.2d", listOfSynthTypes[i], j); // the .2 makes banks have leading zero and always use two characters 01 to 10
      if (!SD.exists(dirName)) SD.mkdir(dirName);
    }
  } 
}
 
Note that %2d or %.2d as a format string doesn't limit the length of its expansion to 2 characters. So allocate
enough characters in the buffer for the largest integer plus a few more - even if you think that can never happen.

Otherwise you make debugging a whole lot harder when your assumptions turn out to be wrong, as one issue
may trigger another memory overwrite like this, and you'll end up chasing your tail - I recommend programming
defensively for peace of mind!
 
I second what @MarkT mentioned. I am old timer and forgot they now allow variables to be allocated on the stack which are not fixed sized...
So I would have probably done something like simply allocate it some length like 16 bytes... And again still use something like snprintf as it will if necessary truncate your string versus overwrite and corrupt memory.
 
+1 on never using sprintf(); use snprintf().

> for (int i = 0; i < numOflistOfSynthTypes; i++) {

Don't bury array sizes down in the code. In most cases, should be more like:

for (auto s : listOfSynthTypes) ....


Also get in the habit of using assert() statements to explicitly state and test your assumptions.
 
Thanks all, I'll increase all my char sizes, switch to using snprint(f) and that auto range-based looping looks really cool, I hadn't come across that before :)
 
Status
Not open for further replies.
Back
Top