Forum Rule: Always post complete source code & details to reproduce any issue!
Results 1 to 9 of 9

Thread: My variable is getting corrupted

  1. #1
    Member
    Join Date
    Jul 2019
    Location
    United Kingdom
    Posts
    21

    My variable is getting corrupted

    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 Code:
    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);
        }
      }
    }

  2. #2
    Senior Member+ KurtE's Avatar
    Join Date
    Jan 2014
    Posts
    7,692
    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.

  3. #3
    Senior Member brtaylor's Avatar
    Join Date
    Mar 2016
    Location
    Portland, OR
    Posts
    580
    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.

  4. #4
    Member
    Join Date
    Jul 2019
    Location
    United Kingdom
    Posts
    21
    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);
        }
      } 
    }

  5. #5
    Member
    Join Date
    Jul 2019
    Location
    United Kingdom
    Posts
    21
    magic, many thanks brtaylor

  6. #6
    Senior Member
    Join Date
    Jul 2020
    Posts
    406
    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!

  7. #7
    Senior Member+ KurtE's Avatar
    Join Date
    Jan 2014
    Posts
    7,692
    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.

  8. #8
    Senior Member
    Join Date
    May 2015
    Location
    USA
    Posts
    650
    +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.

  9. #9
    Member
    Join Date
    Jul 2019
    Location
    United Kingdom
    Posts
    21
    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

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •