| Message ID | 20220427230716.2158127-1-pcc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | loader: support loading large files (>=2GB) | expand |
On 4/27/22 16:07, Peter Collingbourne wrote: > Currently the loader uses int as the return type for various APIs > that deal with file sizes, which leads to an error if the file > size is >=2GB, as it ends up being interpreted as a negative error > code. Furthermore, we do not tolerate short reads, which are possible > at least on Linux when attempting to read such large files in one > syscall. > > Fix the first problem by switching to 64-bit types for file sizes, > and fix the second by introducing a loop around the read syscall. > > Signed-off-by: Peter Collingbourne<pcc@google.com> > --- > hw/core/generic-loader.c | 2 +- > hw/core/loader.c | 44 ++++++++++++++++++++++++---------------- > include/hw/loader.h | 13 ++++++------ > 3 files changed, 34 insertions(+), 25 deletions(-) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
Hi Peter, On 28/4/22 01:07, Peter Collingbourne wrote: > Currently the loader uses int as the return type for various APIs > that deal with file sizes, which leads to an error if the file > size is >=2GB, as it ends up being interpreted as a negative error > code. Furthermore, we do not tolerate short reads, which are possible > at least on Linux when attempting to read such large files in one > syscall. > > Fix the first problem by switching to 64-bit types for file sizes, > and fix the second by introducing a loop around the read syscall. Hmm maybe worth rebasing on this patch from Jamie which also fixes the format on 32-bit hosts: https://lore.kernel.org/qemu-devel/20211111141141.3295094-2-jamie@nuviainc.com/ (Personally I prefer to read ssize_t while reviewing instead of int64_t). While here, please have a look at this other patch from Jamie: https://lore.kernel.org/qemu-devel/20211111141141.3295094-3-jamie@nuviainc.com/ Also, Cc'ing the maintainer: $ ./scripts/get_maintainer.pl -f hw/core/generic-loader.c Alistair Francis <alistair@alistair23.me> (maintainer:Generic Loader) > Signed-off-by: Peter Collingbourne <pcc@google.com> > --- > hw/core/generic-loader.c | 2 +- > hw/core/loader.c | 44 ++++++++++++++++++++++++---------------- > include/hw/loader.h | 13 ++++++------ > 3 files changed, 34 insertions(+), 25 deletions(-) > > diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c > index c666545aa0..0891fa73c3 100644 > --- a/hw/core/generic-loader.c > +++ b/hw/core/generic-loader.c > @@ -67,7 +67,7 @@ static void generic_loader_realize(DeviceState *dev, Error **errp) > GenericLoaderState *s = GENERIC_LOADER(dev); > hwaddr entry; > int big_endian; > - int size = 0; > + int64_t size = 0; > > s->set_pc = false; > > diff --git a/hw/core/loader.c b/hw/core/loader.c > index ca2f2431fb..d07c79c400 100644 > --- a/hw/core/loader.c > +++ b/hw/core/loader.c > @@ -115,17 +115,17 @@ ssize_t read_targphys(const char *name, > return did; > } > > -int load_image_targphys(const char *filename, > - hwaddr addr, uint64_t max_sz) > +int64_t load_image_targphys(const char *filename, > + hwaddr addr, uint64_t max_sz) > { > return load_image_targphys_as(filename, addr, max_sz, NULL); > } > > /* return the size or -1 if error */ > -int load_image_targphys_as(const char *filename, > - hwaddr addr, uint64_t max_sz, AddressSpace *as) > +int64_t load_image_targphys_as(const char *filename, > + hwaddr addr, uint64_t max_sz, AddressSpace *as) > { > - int size; > + int64_t size; > > size = get_image_size(filename); > if (size < 0 || size > max_sz) { > @@ -139,9 +139,9 @@ int load_image_targphys_as(const char *filename, > return size; > } > > -int load_image_mr(const char *filename, MemoryRegion *mr) > +int64_t load_image_mr(const char *filename, MemoryRegion *mr) > { > - int size; > + int64_t size; > > if (!memory_access_is_direct(mr, false)) { > /* Can only load an image into RAM or ROM */ > @@ -963,7 +963,8 @@ int rom_add_file(const char *file, const char *fw_dir, > { > MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine()); > Rom *rom; > - int rc, fd = -1; > + int fd = -1; > + size_t bytes_read = 0; > char devpath[100]; > > if (as && mr) { > @@ -1003,11 +1004,17 @@ int rom_add_file(const char *file, const char *fw_dir, > rom->datasize = rom->romsize; > rom->data = g_malloc0(rom->datasize); > lseek(fd, 0, SEEK_SET); > - rc = read(fd, rom->data, rom->datasize); > - if (rc != rom->datasize) { > - fprintf(stderr, "rom: file %-20s: read error: rc=%d (expected %zd)\n", > - rom->name, rc, rom->datasize); > - goto err; > + while (bytes_read < rom->datasize) { > + ssize_t rc = > + read(fd, rom->data + bytes_read, rom->datasize - bytes_read); > + if (rc <= 0) { > + fprintf(stderr, > + "rom: file %-20s: read error: rc=%zd at position %zd " > + "(expected size %zd)\n", > + rom->name, rc, bytes_read, rom->datasize); > + goto err; > + } > + bytes_read += rc; > } > close(fd); > rom_insert(rom); > @@ -1671,7 +1678,7 @@ typedef struct { > HexLine line; > uint8_t *bin_buf; > hwaddr *start_addr; > - int total_size; > + int64_t total_size; > uint32_t next_address_to_write; > uint32_t current_address; > uint32_t current_rom_index; > @@ -1767,8 +1774,8 @@ static int handle_record_type(HexParser *parser) > } > > /* return size or -1 if error */ > -static int parse_hex_blob(const char *filename, hwaddr *addr, uint8_t *hex_blob, > - size_t hex_blob_size, AddressSpace *as) > +static int64_t parse_hex_blob(const char *filename, hwaddr *addr, uint8_t *hex_blob, > + size_t hex_blob_size, AddressSpace *as) > { > bool in_process = false; /* avoid re-enter and > * check whether record begin with ':' */ > @@ -1832,11 +1839,12 @@ out: > } > > /* return size or -1 if error */ > -int load_targphys_hex_as(const char *filename, hwaddr *entry, AddressSpace *as) > +int64_t load_targphys_hex_as(const char *filename, hwaddr *entry, > + AddressSpace *as) > { > gsize hex_blob_size; > gchar *hex_blob; > - int total_size = 0; > + int64_t total_size = 0; > > if (!g_file_get_contents(filename, &hex_blob, &hex_blob_size, NULL)) { > return -1; > diff --git a/include/hw/loader.h b/include/hw/loader.h > index 5572108ba5..7b09705940 100644 > --- a/include/hw/loader.h > +++ b/include/hw/loader.h > @@ -40,8 +40,8 @@ ssize_t load_image_size(const char *filename, void *addr, size_t size); > * > * Returns the size of the loaded image on success, -1 otherwise. > */ > -int load_image_targphys_as(const char *filename, > - hwaddr addr, uint64_t max_sz, AddressSpace *as); > +int64_t load_image_targphys_as(const char *filename, > + hwaddr addr, uint64_t max_sz, AddressSpace *as); > > /**load_targphys_hex_as: > * @filename: Path to the .hex file > @@ -53,14 +53,15 @@ int load_image_targphys_as(const char *filename, > * > * Returns the size of the loaded .hex file on success, -1 otherwise. > */ > -int load_targphys_hex_as(const char *filename, hwaddr *entry, AddressSpace *as); > +int64_t load_targphys_hex_as(const char *filename, hwaddr *entry, > + AddressSpace *as); > > /** load_image_targphys: > * Same as load_image_targphys_as(), but doesn't allow the caller to specify > * an AddressSpace. > */ > -int load_image_targphys(const char *filename, hwaddr, > - uint64_t max_sz); > +int64_t load_image_targphys(const char *filename, hwaddr, > + uint64_t max_sz); > > /** > * load_image_mr: load an image into a memory region > @@ -73,7 +74,7 @@ int load_image_targphys(const char *filename, hwaddr, > * If the file is larger than the memory region's size the call will fail. > * Returns -1 on failure, or the size of the file. > */ > -int load_image_mr(const char *filename, MemoryRegion *mr); > +int64_t load_image_mr(const char *filename, MemoryRegion *mr); > > /* This is the limit on the maximum uncompressed image size that > * load_image_gzipped_buffer() and load_image_gzipped() will read. It prevents
On Tue, May 31, 2022 at 12:59 AM Philippe Mathieu-Daudé via <qemu-devel@nongnu.org> wrote: > > Hi Peter, > > On 28/4/22 01:07, Peter Collingbourne wrote: > > Currently the loader uses int as the return type for various APIs > > that deal with file sizes, which leads to an error if the file > > size is >=2GB, as it ends up being interpreted as a negative error > > code. Furthermore, we do not tolerate short reads, which are possible > > at least on Linux when attempting to read such large files in one > > syscall. > > > > Fix the first problem by switching to 64-bit types for file sizes, > > and fix the second by introducing a loop around the read syscall. > > Hmm maybe worth rebasing on this patch from Jamie which also > fixes the format on 32-bit hosts: > https://lore.kernel.org/qemu-devel/20211111141141.3295094-2-jamie@nuviainc.com/ > > (Personally I prefer to read ssize_t while reviewing instead > of int64_t). I agree with ssize_t as well, I have applied the patch from Jamie which had fallen through the cracks. If you can rebase this on top of the RISC-V queue and re-send it I'll apply the other changes Alistair > > While here, please have a look at this other patch from Jamie: > https://lore.kernel.org/qemu-devel/20211111141141.3295094-3-jamie@nuviainc.com/ > > Also, Cc'ing the maintainer: > > $ ./scripts/get_maintainer.pl -f hw/core/generic-loader.c > Alistair Francis <alistair@alistair23.me> (maintainer:Generic Loader) > > > Signed-off-by: Peter Collingbourne <pcc@google.com> > > --- > > hw/core/generic-loader.c | 2 +- > > hw/core/loader.c | 44 ++++++++++++++++++++++++---------------- > > include/hw/loader.h | 13 ++++++------ > > 3 files changed, 34 insertions(+), 25 deletions(-) > > > > diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c > > index c666545aa0..0891fa73c3 100644 > > --- a/hw/core/generic-loader.c > > +++ b/hw/core/generic-loader.c > > @@ -67,7 +67,7 @@ static void generic_loader_realize(DeviceState *dev, Error **errp) > > GenericLoaderState *s = GENERIC_LOADER(dev); > > hwaddr entry; > > int big_endian; > > - int size = 0; > > + int64_t size = 0; > > > > s->set_pc = false; > > > > diff --git a/hw/core/loader.c b/hw/core/loader.c > > index ca2f2431fb..d07c79c400 100644 > > --- a/hw/core/loader.c > > +++ b/hw/core/loader.c > > @@ -115,17 +115,17 @@ ssize_t read_targphys(const char *name, > > return did; > > } > > > > -int load_image_targphys(const char *filename, > > - hwaddr addr, uint64_t max_sz) > > +int64_t load_image_targphys(const char *filename, > > + hwaddr addr, uint64_t max_sz) > > { > > return load_image_targphys_as(filename, addr, max_sz, NULL); > > } > > > > /* return the size or -1 if error */ > > -int load_image_targphys_as(const char *filename, > > - hwaddr addr, uint64_t max_sz, AddressSpace *as) > > +int64_t load_image_targphys_as(const char *filename, > > + hwaddr addr, uint64_t max_sz, AddressSpace *as) > > { > > - int size; > > + int64_t size; > > > > size = get_image_size(filename); > > if (size < 0 || size > max_sz) { > > @@ -139,9 +139,9 @@ int load_image_targphys_as(const char *filename, > > return size; > > } > > > > -int load_image_mr(const char *filename, MemoryRegion *mr) > > +int64_t load_image_mr(const char *filename, MemoryRegion *mr) > > { > > - int size; > > + int64_t size; > > > > if (!memory_access_is_direct(mr, false)) { > > /* Can only load an image into RAM or ROM */ > > @@ -963,7 +963,8 @@ int rom_add_file(const char *file, const char *fw_dir, > > { > > MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine()); > > Rom *rom; > > - int rc, fd = -1; > > + int fd = -1; > > + size_t bytes_read = 0; > > char devpath[100]; > > > > if (as && mr) { > > @@ -1003,11 +1004,17 @@ int rom_add_file(const char *file, const char *fw_dir, > > rom->datasize = rom->romsize; > > rom->data = g_malloc0(rom->datasize); > > lseek(fd, 0, SEEK_SET); > > - rc = read(fd, rom->data, rom->datasize); > > - if (rc != rom->datasize) { > > - fprintf(stderr, "rom: file %-20s: read error: rc=%d (expected %zd)\n", > > - rom->name, rc, rom->datasize); > > - goto err; > > + while (bytes_read < rom->datasize) { > > + ssize_t rc = > > + read(fd, rom->data + bytes_read, rom->datasize - bytes_read); > > + if (rc <= 0) { > > + fprintf(stderr, > > + "rom: file %-20s: read error: rc=%zd at position %zd " > > + "(expected size %zd)\n", > > + rom->name, rc, bytes_read, rom->datasize); > > + goto err; > > + } > > + bytes_read += rc; > > } > > close(fd); > > rom_insert(rom); > > @@ -1671,7 +1678,7 @@ typedef struct { > > HexLine line; > > uint8_t *bin_buf; > > hwaddr *start_addr; > > - int total_size; > > + int64_t total_size; > > uint32_t next_address_to_write; > > uint32_t current_address; > > uint32_t current_rom_index; > > @@ -1767,8 +1774,8 @@ static int handle_record_type(HexParser *parser) > > } > > > > /* return size or -1 if error */ > > -static int parse_hex_blob(const char *filename, hwaddr *addr, uint8_t *hex_blob, > > - size_t hex_blob_size, AddressSpace *as) > > +static int64_t parse_hex_blob(const char *filename, hwaddr *addr, uint8_t *hex_blob, > > + size_t hex_blob_size, AddressSpace *as) > > { > > bool in_process = false; /* avoid re-enter and > > * check whether record begin with ':' */ > > @@ -1832,11 +1839,12 @@ out: > > } > > > > /* return size or -1 if error */ > > -int load_targphys_hex_as(const char *filename, hwaddr *entry, AddressSpace *as) > > +int64_t load_targphys_hex_as(const char *filename, hwaddr *entry, > > + AddressSpace *as) > > { > > gsize hex_blob_size; > > gchar *hex_blob; > > - int total_size = 0; > > + int64_t total_size = 0; > > > > if (!g_file_get_contents(filename, &hex_blob, &hex_blob_size, NULL)) { > > return -1; > > diff --git a/include/hw/loader.h b/include/hw/loader.h > > index 5572108ba5..7b09705940 100644 > > --- a/include/hw/loader.h > > +++ b/include/hw/loader.h > > @@ -40,8 +40,8 @@ ssize_t load_image_size(const char *filename, void *addr, size_t size); > > * > > * Returns the size of the loaded image on success, -1 otherwise. > > */ > > -int load_image_targphys_as(const char *filename, > > - hwaddr addr, uint64_t max_sz, AddressSpace *as); > > +int64_t load_image_targphys_as(const char *filename, > > + hwaddr addr, uint64_t max_sz, AddressSpace *as); > > > > /**load_targphys_hex_as: > > * @filename: Path to the .hex file > > @@ -53,14 +53,15 @@ int load_image_targphys_as(const char *filename, > > * > > * Returns the size of the loaded .hex file on success, -1 otherwise. > > */ > > -int load_targphys_hex_as(const char *filename, hwaddr *entry, AddressSpace *as); > > +int64_t load_targphys_hex_as(const char *filename, hwaddr *entry, > > + AddressSpace *as); > > > > /** load_image_targphys: > > * Same as load_image_targphys_as(), but doesn't allow the caller to specify > > * an AddressSpace. > > */ > > -int load_image_targphys(const char *filename, hwaddr, > > - uint64_t max_sz); > > +int64_t load_image_targphys(const char *filename, hwaddr, > > + uint64_t max_sz); > > > > /** > > * load_image_mr: load an image into a memory region > > @@ -73,7 +74,7 @@ int load_image_targphys(const char *filename, hwaddr, > > * If the file is larger than the memory region's size the call will fail. > > * Returns -1 on failure, or the size of the file. > > */ > > -int load_image_mr(const char *filename, MemoryRegion *mr); > > +int64_t load_image_mr(const char *filename, MemoryRegion *mr); > > > > /* This is the limit on the maximum uncompressed image size that > > * load_image_gzipped_buffer() and load_image_gzipped() will read. It prevents > >
diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c index c666545aa0..0891fa73c3 100644 --- a/hw/core/generic-loader.c +++ b/hw/core/generic-loader.c @@ -67,7 +67,7 @@ static void generic_loader_realize(DeviceState *dev, Error **errp) GenericLoaderState *s = GENERIC_LOADER(dev); hwaddr entry; int big_endian; - int size = 0; + int64_t size = 0; s->set_pc = false; diff --git a/hw/core/loader.c b/hw/core/loader.c index ca2f2431fb..d07c79c400 100644 --- a/hw/core/loader.c +++ b/hw/core/loader.c @@ -115,17 +115,17 @@ ssize_t read_targphys(const char *name, return did; } -int load_image_targphys(const char *filename, - hwaddr addr, uint64_t max_sz) +int64_t load_image_targphys(const char *filename, + hwaddr addr, uint64_t max_sz) { return load_image_targphys_as(filename, addr, max_sz, NULL); } /* return the size or -1 if error */ -int load_image_targphys_as(const char *filename, - hwaddr addr, uint64_t max_sz, AddressSpace *as) +int64_t load_image_targphys_as(const char *filename, + hwaddr addr, uint64_t max_sz, AddressSpace *as) { - int size; + int64_t size; size = get_image_size(filename); if (size < 0 || size > max_sz) { @@ -139,9 +139,9 @@ int load_image_targphys_as(const char *filename, return size; } -int load_image_mr(const char *filename, MemoryRegion *mr) +int64_t load_image_mr(const char *filename, MemoryRegion *mr) { - int size; + int64_t size; if (!memory_access_is_direct(mr, false)) { /* Can only load an image into RAM or ROM */ @@ -963,7 +963,8 @@ int rom_add_file(const char *file, const char *fw_dir, { MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine()); Rom *rom; - int rc, fd = -1; + int fd = -1; + size_t bytes_read = 0; char devpath[100]; if (as && mr) { @@ -1003,11 +1004,17 @@ int rom_add_file(const char *file, const char *fw_dir, rom->datasize = rom->romsize; rom->data = g_malloc0(rom->datasize); lseek(fd, 0, SEEK_SET); - rc = read(fd, rom->data, rom->datasize); - if (rc != rom->datasize) { - fprintf(stderr, "rom: file %-20s: read error: rc=%d (expected %zd)\n", - rom->name, rc, rom->datasize); - goto err; + while (bytes_read < rom->datasize) { + ssize_t rc = + read(fd, rom->data + bytes_read, rom->datasize - bytes_read); + if (rc <= 0) { + fprintf(stderr, + "rom: file %-20s: read error: rc=%zd at position %zd " + "(expected size %zd)\n", + rom->name, rc, bytes_read, rom->datasize); + goto err; + } + bytes_read += rc; } close(fd); rom_insert(rom); @@ -1671,7 +1678,7 @@ typedef struct { HexLine line; uint8_t *bin_buf; hwaddr *start_addr; - int total_size; + int64_t total_size; uint32_t next_address_to_write; uint32_t current_address; uint32_t current_rom_index; @@ -1767,8 +1774,8 @@ static int handle_record_type(HexParser *parser) } /* return size or -1 if error */ -static int parse_hex_blob(const char *filename, hwaddr *addr, uint8_t *hex_blob, - size_t hex_blob_size, AddressSpace *as) +static int64_t parse_hex_blob(const char *filename, hwaddr *addr, uint8_t *hex_blob, + size_t hex_blob_size, AddressSpace *as) { bool in_process = false; /* avoid re-enter and * check whether record begin with ':' */ @@ -1832,11 +1839,12 @@ out: } /* return size or -1 if error */ -int load_targphys_hex_as(const char *filename, hwaddr *entry, AddressSpace *as) +int64_t load_targphys_hex_as(const char *filename, hwaddr *entry, + AddressSpace *as) { gsize hex_blob_size; gchar *hex_blob; - int total_size = 0; + int64_t total_size = 0; if (!g_file_get_contents(filename, &hex_blob, &hex_blob_size, NULL)) { return -1; diff --git a/include/hw/loader.h b/include/hw/loader.h index 5572108ba5..7b09705940 100644 --- a/include/hw/loader.h +++ b/include/hw/loader.h @@ -40,8 +40,8 @@ ssize_t load_image_size(const char *filename, void *addr, size_t size); * * Returns the size of the loaded image on success, -1 otherwise. */ -int load_image_targphys_as(const char *filename, - hwaddr addr, uint64_t max_sz, AddressSpace *as); +int64_t load_image_targphys_as(const char *filename, + hwaddr addr, uint64_t max_sz, AddressSpace *as); /**load_targphys_hex_as: * @filename: Path to the .hex file @@ -53,14 +53,15 @@ int load_image_targphys_as(const char *filename, * * Returns the size of the loaded .hex file on success, -1 otherwise. */ -int load_targphys_hex_as(const char *filename, hwaddr *entry, AddressSpace *as); +int64_t load_targphys_hex_as(const char *filename, hwaddr *entry, + AddressSpace *as); /** load_image_targphys: * Same as load_image_targphys_as(), but doesn't allow the caller to specify * an AddressSpace. */ -int load_image_targphys(const char *filename, hwaddr, - uint64_t max_sz); +int64_t load_image_targphys(const char *filename, hwaddr, + uint64_t max_sz); /** * load_image_mr: load an image into a memory region @@ -73,7 +74,7 @@ int load_image_targphys(const char *filename, hwaddr, * If the file is larger than the memory region's size the call will fail. * Returns -1 on failure, or the size of the file. */ -int load_image_mr(const char *filename, MemoryRegion *mr); +int64_t load_image_mr(const char *filename, MemoryRegion *mr); /* This is the limit on the maximum uncompressed image size that * load_image_gzipped_buffer() and load_image_gzipped() will read. It prevents
Currently the loader uses int as the return type for various APIs that deal with file sizes, which leads to an error if the file size is >=2GB, as it ends up being interpreted as a negative error code. Furthermore, we do not tolerate short reads, which are possible at least on Linux when attempting to read such large files in one syscall. Fix the first problem by switching to 64-bit types for file sizes, and fix the second by introducing a loop around the read syscall. Signed-off-by: Peter Collingbourne <pcc@google.com> --- hw/core/generic-loader.c | 2 +- hw/core/loader.c | 44 ++++++++++++++++++++++++---------------- include/hw/loader.h | 13 ++++++------ 3 files changed, 34 insertions(+), 25 deletions(-)