| Message ID | 9c0be954b608da7d47c6a0c120da332c16798d2a.1718195956.git.amjadsharafi10@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | vvfat: Fix write bugs for large files and add iotests | expand |
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? > + (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
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 >
On Jul 19 2024, at 8:20 am, Amjad Alsharafi <amjadsharafi10@gmail.com> wrote: > 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 Should I send a new patch? since most commits are reviewed now > >> >> > + (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 >> >
Am 19.07.2024 um 02:29 hat Amjad Alsharafi geschrieben: > > > On Jul 19 2024, at 8:20 am, Amjad Alsharafi <amjadsharafi10@gmail.com> wrote: > > > 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 > > Should I send a new patch? since most commits are reviewed now Yes, please do. I think I reviewed the whole series. Kevin > > > >> > >> > + (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 > >> > > >
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 || + (new_path = strcmp(s->current_mapping->path, mapping->path))) { + + if (new_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; } return 0;
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(-)