On 4/30/21 11:25 AM, Philippe Mathieu-Daudé wrote: > The direntry_t::name holds 11 bytes: > > typedef struct direntry_t { > uint8_t name[8 + 3]; > ...
struct direntry_t is poorly laid out. A quick google search finds: https://www.ntfs.com/fat-filenames.htm which shows that a long file name is indeed split over multiple fields of struct direntry_t. In addition to the direntry_t for the 8+3 short name, there are additional direntry_t reserved for each 13 bytes of the long name (well, 26 bytes of UTF-16, as those 13 bytes are expanded into Unicode). > > However create_long_filename() writes up to 31 bytes into it: > > 421 for(i=0;i<26*number_of_entries;i++) { > 422 int offset=(i%26); > 423 if(offset<10) offset=1+offset; > 424 else if(offset<22) offset=14+offset-10; > 425 else offset=28+offset-22; > 426 entry=array_get(&(s->directory),s->directory.next-1-(i/26)); > 427 if (i >= 2 * length + 2) { > 428 entry->name[offset] = 0xff; > 429 } else if (i % 2 == 0) { > 430 entry->name[offset] = longname[i / 2] & 0xff; > 431 } else { > 432 entry->name[offset] = longname[i / 2] >> 8; > 433 } > 434 } The code is probably correct for which bytes it is writing, but wrong for calling it entry->name[offset]; what we probably want is: typedef struct direntry_t { union { struct { uint8_t name[8 + 3]; ... uint32_t size; }; char raw[32]; }; } QEMU_PACKED direntry_t; where we then access through entry->raw[offset] instead of entry->name[offset] (I know that anonymous union/structs are not standard C, but I think that both gcc and clang support them, so that we don't have to refactor the rest of the code to go through additional names introduced by those modifications to direntry_t). > > For example, if i=25, offset=28+25-22=31 > > Then in lines 428, 430 and 432 the entry->name[] array is written beside > its 11 bytes, as reported by Clang sanitizer: > > block/vvfat.c:430:13: runtime error: index 14 out of bounds for type > 'uint8_t [11]' > SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior > block/vvfat.c:430:13 in > block/vvfat.c:432:13: runtime error: index 15 out of bounds for type > 'uint8_t [11]' > SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior > block/vvfat.c:432:13 in > block/vvfat.c:428:13: runtime error: index 18 out of bounds for type > 'uint8_t [11]' > SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior > block/vvfat.c:428:13 in > > As I have no idea about what this code does, simply skip the writes if > out of range, since it is not worst than what we have currently (and > my tests using vvfat work identically). > > Fixes: de167e416fa ("Virtual VFAT support (Johannes Schindelin)") > Signed-off-by: Philippe Mathieu-Daudé <phi...@redhat.com> NACK. This violates what FAT expects for long names. Our code is not pretty, but we should clean it up correctly rather than breaking it. > --- > block/vvfat.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/block/vvfat.c b/block/vvfat.c > index c193a816646..c7162e77d68 100644 > --- a/block/vvfat.c > +++ b/block/vvfat.c > @@ -423,6 +423,9 @@ static direntry_t *create_long_filename(BDRVVVFATState > *s, const char *filename) > if(offset<10) offset=1+offset; > else if(offset<22) offset=14+offset-10; > else offset=28+offset-22; > + if (offset >= ARRAY_SIZE(entry->name)) { > + continue; > + } > entry=array_get(&(s->directory),s->directory.next-1-(i/26)); > if (i >= 2 * length + 2) { > entry->name[offset] = 0xff; > -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org