Re: [PATCH] vvfat: refresh writing long filename

[BALATON Zoltan]

On Sat, 18 Jan 2025, Michael Tokarev wrote:
> 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??melin.
>
> Signed-off-by: Michael Tokarev <mjt@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;

Why not uint16_t and maybe cpu_to_le (or whatever that's called) if 
needed? May be simpler than handling it byte by byte.

Regards,
BALATON Zoltan

> +    }
> +    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);
>     }
>     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);
> }
>
> 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++;
>         }
>     }