Am 18.01.25 um 18:35 schrieb Michael Tokarev:
> In function create_long_filname(), the array name[8 + 3] in
> struct direntry_t is used as if it were defined as name[32].
> This is intentional and works. It's nevertheless an out of
> bounds array access. To avoid this problem, this patch adds a
> struct lfn_direntry_t with multiple name arrays. A directory
> entry for a long FAT file name is significantly different from
> a directory entry for a regular FAT file name.
>
> This change makes whole logic dealing with the long filenames
> a bit more clear (hopefully).
>
> Based on ideas by Volker R\ufffd\ufffdmelin.
>
> Signed-off-by: Michael Tokarev <m...@tls.msk.ru>
> ---
> block/vvfat.c | 69 ++++++++++++++++++++++++++++++++-------------------
> 1 file changed, 43 insertions(+), 26 deletions(-)
>
> diff --git a/block/vvfat.c b/block/vvfat.c
> index 8ffe8b3b9b..8320733045 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -255,6 +255,17 @@ typedef struct direntry_t {
> uint32_t size;
> } QEMU_PACKED direntry_t;
>
> +typedef struct lfn_direntry_t {
> + uint8_t sequence;
> + uint8_t name01[10];
> + uint8_t attributes;
> + uint8_t direntry_type;
> + uint8_t sfn_checksum;
> + uint8_t name0e[12];
> + uint16_t begin;
> + uint8_t name1c[4];
> +} QEMU_PACKED lfn_direntry_t;
> +
> /* this structure are used to transparently access the files */
>
> typedef struct mapping_t {
> @@ -399,11 +410,22 @@ static void init_mbr(BDRVVVFATState *s, int cyls, int
> heads, int secs)
>
> /* direntry functions */
>
> +static unsigned write_lfn_part(uint8_t *dest, unsigned dsize,
> + const gunichar2 *lptr, const gunichar2 *lend)
> +{
> + unsigned i;
> + for(i = 0; i < dsize / 2 && lptr + i < lend; ++i) {
> + dest[i / 2 + 0] = lptr[i] & 0xff;
> + dest[i / 2 + 1] = lptr[i] >> 8;
> + }
Hi Michael,
this is not right. It's necessary to initialize the remaining elements
of the name arrays.
The rules are:
If the file name length in characters is a multiple of 13 you are done.
Otherwise the remaining unused LFN direntry name array elements have to
be filled with one 0x0000 (a 16 bit 0) and the rest with 0xffff.
> + return i; /* return number of elements actually written */
> +}
> +
> static direntry_t *create_long_filename(BDRVVVFATState *s, const char
> *filename)
> {
> - int number_of_entries, i;
> + unsigned number_of_entries, entidx;
> + gunichar2 *lptr, *lend;
> glong length;
> - direntry_t *entry;
>
> gunichar2 *longname = g_utf8_to_utf16(filename, -1, NULL, &length, NULL);
> if (!longname) {
> @@ -411,31 +433,26 @@ static direntry_t *create_long_filename(BDRVVVFATState
> *s, const char *filename)
> return NULL;
> }
>
> - number_of_entries = DIV_ROUND_UP(length * 2, 26);
> -
> - for(i=0;i<number_of_entries;i++) {
> - entry=array_get_next(&(s->directory));
> - entry->attributes=0xf;
> - entry->reserved[0]=0;
> - entry->begin=0;
> - entry->name[0]=(number_of_entries-i)|(i==0?0x40:0);
> - }
> - for(i=0;i<26*number_of_entries;i++) {
> - int offset=(i%26);
> - if(offset<10) offset=1+offset;
> - else if(offset<22) offset=14+offset-10;
> - else offset=28+offset-22;
> - entry=array_get(&(s->directory),s->directory.next-1-(i/26));
> - if (i >= 2 * length + 2) {
> - entry->name[offset] = 0xff;
> - } else if (i % 2 == 0) {
> - entry->name[offset] = longname[i / 2] & 0xff;
> - } else {
> - entry->name[offset] = longname[i / 2] >> 8;
> - }
> + /*
> + * each lfn_direntry holds 13 utf16 chars (26 bytes) of the file name,
> + * the name is split into several directory entries.
> + */
> + number_of_entries = DIV_ROUND_UP(length, 13);
> +
> + lend = longname + length; /* pointer past the end of longname */
> + for(entidx = 0, lptr = longname; entidx < number_of_entries; entidx++) {
> + lfn_direntry_t *entry = array_get_next(&(s->directory));
> + entry->sequence = (number_of_entries - entidx) | (entidx == 0 ? 0x40
> : 0);
> + entry->attributes = 0xf;
> + entry->direntry_type = 0;
> + entry->begin = 0;
> + /* each lftn_direntry has 3 pieces to hold parts of long file name */
> + lptr += write_lfn_part(entry->name01, sizeof(entry->name01), lptr,
> lend);
> + lptr += write_lfn_part(entry->name0e, sizeof(entry->name0e), lptr,
> lend);
> + lptr += write_lfn_part(entry->name1c, sizeof(entry->name1c), lptr,
> lend);
I would use ARRAY_SIZE() instead of sizeof(). ARRAY_SIZE() is the number
of elements an array can hold. You then don't have to remember the size
of an array element.
> }
> g_free(longname);
> - return array_get(&(s->directory),s->directory.next-number_of_entries);
> + return array_get(&(s->directory), s->directory.next - number_of_entries);
+ return array_get(&s->directory, s->directory.next - number_of_entries);
The parentheses around s->directory are unnecessary.
With best regards,
Volker
> }
>
> static char is_free(const direntry_t* direntry)
> @@ -731,7 +748,7 @@ static inline direntry_t*
> create_short_and_long_name(BDRVVVFATState* s,
> /* calculate anew, because realloc could have taken place */
> entry_long=array_get(&(s->directory),long_index);
> while(entry_long<entry && is_long_name(entry_long)) {
> - entry_long->reserved[1]=chksum;
> + ((lfn_direntry_t *)entry_long)->sfn_checksum = chksum;
> entry_long++;
> }
> }
