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

[Amjad Alsharafi <amjadsharafi10@gmail.com>]

On Thu, Jul 18, 2024 at 05:20:36PM +0200, Kevin Wolf wrote:
> Am 12.06.2024 um 14:43 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 | 23 ++++++++++++++++-------
> >  1 file changed, 16 insertions(+), 7 deletions(-)
> > 
> > diff --git a/block/vvfat.c b/block/vvfat.c
> > index b63ac5d045..fc570d0610 100644
> > --- a/block/vvfat.c
> > +++ b/block/vvfat.c
> > @@ -1360,15 +1360,24 @@ static int open_file(BDRVVVFATState* s,mapping_t* mapping)
> >  {
> >      if(!mapping)
> >          return -1;
> > +    int new_path = 1;
> >      if(!s->current_mapping ||
> > -            strcmp(s->current_mapping->path,mapping->path)) {
> > -        /* open file */
> > -        int fd = qemu_open_old(mapping->path,
> > +            s->current_mapping->info.file.offset
> > +                != mapping->info.file.offset ||
> 
> I'm wondering if this couldn't just be s->current_mapping != mapping?

Actually, you are totally right. Not sure what made me go for this.

I tried also to test with only checking if the path changed, but it
fails on some tests. So the offset is important.
For that reason, checking just the mapping ptr is better since we won't
have 2 mappings with same file and offset.

I'll then use this change. Thanks

> 
> > +            (new_path = strcmp(s->current_mapping->path, mapping->path))) {
> 
> If both the path and the offset change, we still want to set new_path, I
> think. And if we didn't already have a mapping, we also need to open the
> file.
> 
> Actually, setting a variable inside the condition makes it kind of hard
> to read, so if s->current_mapping != mapping works, we can do the check
> only in the conditon below:
> 
> > +        if (new_path) {
> 
> if (!s->current_mapping ||
>     strcmp(s->current_mapping->path, mapping->path))
> 
> > +            /* open file */
> > +            int fd = qemu_open_old(mapping->path,
> >                                 O_RDONLY | O_BINARY | O_LARGEFILE);
> > -        if(fd<0)
> > -            return -1;
> > -        vvfat_close_current_file(s);
> > -        s->current_fd = fd;
> > +            if (fd < 0) {
> > +                return -1;
> > +            }
> > +            vvfat_close_current_file(s);
> > +
> > +            s->current_fd = fd;
> > +        }
> > +        assert(s->current_fd);
> >          s->current_mapping = mapping;
> >      }
> 
> Kevin
>