Re: [PATCH v6 4/5] vvfat: Fix reading files with non-continuous clusters

[Kevin Wolf <kwolf@redhat.com>]

Am 20.07.2024 um 12:13 hat Amjad Alsharafi geschrieben:
> When reading with `read_cluster` we get the `mapping` with
> `find_mapping_for_cluster` and then we call `open_file` for this
> mapping.
> The issue appear when its the same file, but a second cluster that is
> not immediately after it, imagine clusters `500 -> 503`, this will give
> us 2 mappings one has the range `500..501` and another `503..504`, both
> point to the same file, but different offsets.
> 
> When we don't open the file since the path is the same, we won't assign
> `s->current_mapping` and thus accessing way out of bound of the file.
> 
> From our example above, after `open_file` (that didn't open anything) we
> will get the offset into the file with
> `s->cluster_size*(cluster_num-s->current_mapping->begin)`, which will
> give us `0x2000 * (504-500)`, which is out of bound for this mapping and
> will produce some issues.
> 
> Signed-off-by: Amjad Alsharafi <amjadsharafi10@gmail.com>
> ---
>  block/vvfat.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/block/vvfat.c b/block/vvfat.c
> index b63ac5d045..d2b879705e 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -1360,15 +1360,17 @@ static int open_file(BDRVVVFATState* s,mapping_t* mapping)
>  {
>      if(!mapping)
>          return -1;
> -    if(!s->current_mapping ||
> -            strcmp(s->current_mapping->path,mapping->path)) {
> +    if (s->current_mapping != mapping) {
>          /* open file */
>          int fd = qemu_open_old(mapping->path,
> -                               O_RDONLY | O_BINARY | O_LARGEFILE);
> -        if(fd<0)
> +                            O_RDONLY | O_BINARY | O_LARGEFILE);
> +        if (fd < 0) {
>              return -1;
> +        }
>          vvfat_close_current_file(s);
> +
>          s->current_fd = fd;
> +        assert(s->current_fd);
>          s->current_mapping = mapping;
>      }
>      return 0;

Now we're reopening the file even if the mapping is actually referring
to the same file that was already open. So the result should be correct,
but wasteful in what is probably a common case.

However, this version of the patch simplified things enough that I think
I finally see what we really want:

diff --git a/block/vvfat.c b/block/vvfat.c
index e3a83fbc88..8ffe8b3b9b 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1369,8 +1369,9 @@ static int open_file(BDRVVVFATState* s,mapping_t* mapping)
             return -1;
         vvfat_close_current_file(s);
         s->current_fd = fd;
-        s->current_mapping = mapping;
     }
+
+    s->current_mapping = mapping;
     return 0;
 }

That should be all that is needed, and it passes your test case.

I don't usually do this, but since time is running out for QEMU 9.1,
I'll just replace the code of this patch with the above and go ahead
with that. If you think it's wrong, let me know and we'll fix it on top
of this series.

Kevin