SdFat-beta
SdFat-beta copied to clipboard
lfnGetName might not properly null terminate callerstring
Looks like getName() doesn't always properly null terminate the string it writes into the user's buffer.
Here's the result I'm seeing on a simple program to recursively print directories.
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;
}
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?
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
I haven't noticed any other problem.
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;
}
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.
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.