create_long_filename() intentionally uses direntry_t->name[8+3] array
as a larger array.  This works, but makes statid code analysis tools
unhappy.  The problem here is that a directory entry holding long file
name is significantly different from regular directory entry, and the
name is split into several parts within the entry, not just in regular
8+3 name field.

Treat the entry as array of bytes instead.  This fixes the OOB access
from the compiler/tools PoV, but does not change the resulting code
in any way.

Keep the existing code style.

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 block/vvfat.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

This code is really ugly and insanely ineffecient.
But LFN is also ugly.

I think this one is the best for now.

Another patch might optimize the second loop quite a bit,
add comments (since it is really difficult to understand,
especially since lfn is so weird), and fix coding style.

diff --git a/block/vvfat.c b/block/vvfat.c
index bb80dfb177..b53e3b452b 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -403,7 +403,6 @@ static direntry_t *create_long_filename(BDRVVVFATState *s, const char *filename)
 {
     int number_of_entries, i;
     glong length;
-    direntry_t *entry;
 
     gunichar2 *longname = g_utf8_to_utf16(filename, -1, NULL, &length, NULL);
     if (!longname) {
@@ -414,24 +413,24 @@ static direntry_t *create_long_filename(BDRVVVFATState *s, const char *filename)
     number_of_entries = DIV_ROUND_UP(length * 2, 26);
 
     for(i=0;i<number_of_entries;i++) {
-        entry=array_get_next(&(s->directory));
+        direntry_t *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++) {
+        unsigned char *entry=array_get(&(s->directory),s->directory.next-1-(i/26));
         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;
+            entry[offset] = 0xff;
         } else if (i % 2 == 0) {
-            entry->name[offset] = longname[i / 2] & 0xff;
+            entry[offset] = longname[i / 2] & 0xff;
         } else {
-            entry->name[offset] = longname[i / 2] >> 8;
+            entry[offset] = longname[i / 2] >> 8;
         }
     }
     g_free(longname);
-- 
2.39.5

19.01.2025 12:15, Michael Tokarev wrote:
> create_long_filename() intentionally uses direntry_t->name[8+3] array
> as a larger array.  This works, but makes statid code analysis tools
> unhappy.  The problem here is that a directory entry holding long file
> name is significantly different from regular directory entry, and the
> name is split into several parts within the entry, not just in regular
> 8+3 name field.
> 
> Treat the entry as array of bytes instead.  This fixes the OOB access
> from the compiler/tools PoV, but does not change the resulting code
> in any way.
> 
> Keep the existing code style.

Hi!

Can we either get some R-b's for this fix, or should we apply
the code-rework patch by Volker?  This theme is stalled for quite
some time now, and the fix isn't in the tree yet.

Thanks,

/mjt

> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
>   block/vvfat.c | 11 +++++------
>   1 file changed, 5 insertions(+), 6 deletions(-)
> 
> This code is really ugly and insanely ineffecient.
> But LFN is also ugly.
> 
> I think this one is the best for now.
> 
> Another patch might optimize the second loop quite a bit,
> add comments (since it is really difficult to understand,
> especially since lfn is so weird), and fix coding style.
> 
> diff --git a/block/vvfat.c b/block/vvfat.c
> index bb80dfb177..b53e3b452b 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -403,7 +403,6 @@ static direntry_t *create_long_filename(BDRVVVFATState *s, const char *filename)
>   {
>       int number_of_entries, i;
>       glong length;
> -    direntry_t *entry;
>   
>       gunichar2 *longname = g_utf8_to_utf16(filename, -1, NULL, &length, NULL);
>       if (!longname) {
> @@ -414,24 +413,24 @@ static direntry_t *create_long_filename(BDRVVVFATState *s, const char *filename)
>       number_of_entries = DIV_ROUND_UP(length * 2, 26);
>   
>       for(i=0;i<number_of_entries;i++) {
> -        entry=array_get_next(&(s->directory));
> +        direntry_t *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++) {
> +        unsigned char *entry=array_get(&(s->directory),s->directory.next-1-(i/26));
>           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;
> +            entry[offset] = 0xff;
>           } else if (i % 2 == 0) {
> -            entry->name[offset] = longname[i / 2] & 0xff;
> +            entry[offset] = longname[i / 2] & 0xff;
>           } else {
> -            entry->name[offset] = longname[i / 2] >> 8;
> +            entry[offset] = longname[i / 2] >> 8;
>           }
>       }
>       g_free(longname);