lfnGetName might not properly null terminate callerstring about sdfat-beta

[PaulStoffregen]

I believe the problem is within lfnGetName(), when the filename stored on the FAT volume has a null terminating char.

static size_t lfnGetName(DirLfn_t* ldir, char* name, size_t n) {
  uint8_t i;
  size_t k = 13*((ldir->order & 0X1F) - 1);
  for (i = 0; i < 13; i++) {
    uint16_t c = lfnGetChar(ldir, i);
    if (c == 0 || k >= (n - 1)) {
      k = n - 1;
      break;
    }
    name[k++] = c >= 0X7F ? '?' : c; 
  }
  // Terminate with zero byte.
  name[k] = '\0';
  return k;
}

Inside the loop, when lfnGetChar() returns zero, k is set to n - 1, which causes the null terminating zero to be written at the end of the user's buffer for the case of short string. The c == 0 case should probably break from the loop without altering k, so the null terminating zero is written at the correct location.

    if (c == 0) break;  // do not alter k for this case, so the zero termination is written at correct location
    if (k >= (n - 1)) {
      k = n - 1;
      break;
    }

[PaulStoffregen]

Or maybe k should be checked first? I'm not sure if the initial compute of k could be too large, or if that's a non-issue?

[greiman]

I need to rethink how getName works. I suspect there is more than one bug. I think I produced a new bug trying to fix another bug.

Long file names are stored like this in the directory. So I am backing though the directory file.

Nth Long entry with flag LAST_LONG_ENTRY (0x40) 
… Additional Long Entries …
1st Long entry 1
Short Entry Associated With Preceding Long Entries

[PaulStoffregen]

I haven't noticed any other problem.

[greiman]

I verified the bug and the following seems to fix it. I would appreciate if you tested any case you have.

static size_t lfnGetName(DirLfn_t* ldir, char* name, size_t n) {
  uint8_t i;
  size_t k = 13*((ldir->order & 0X1F) - 1);
  for (i = 0; i < 13; i++) {
    uint16_t c = lfnGetChar(ldir, i);
    if (c == 0 || k >= (n - 1)) {
     //       k = n - 1;   <<-------removed
      break;
    }
    name[k++] = c >= 0X7F ? '?' : c;
  }
  // Terminate with zero byte.
  if (k >= n) {  // <<----------added
    k = n - 1;   // <<--------- added
  }             // <<---------added
  name[k] = '\0';
  return k;
}

[PaulStoffregen]

Yes, confirmed, that works great.

I tested here by reverting to the original code and checking that the problem did indeed return (turns out not all boards & programs reproduce the problem - many cases default to buffers pre-filled with zeros), and then I put this into FatFileLFN.cpp and it definitely does solve the problem.

[greiman]

Thank for the check. I will post a new beta today.

The bss zeroed array caught me with a quick fix for another case. I now have a sketch that makes all filename lengths, fills the buffer with '?' before each call and uses a series of buffer lengths to verify truncation.