| Message ID | 20190112175812.27068-6-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | nbd: add qemu-nbd --list | expand |
12.01.2019 20:57, Eric Blake wrote: > Although our compile-time environment is set up so that we always > support long files with 64-bit off_t, we have no guarantee whether > off_t is the same type as int64_t. This requires casts when > printing values, and prevents us from directly using qemu_strtoi64(). > Let's just flip to [u]int64_t (signed for length, because we have to > detect failure of blk_getlength() we have not, after previous patch and because off_t was signed; > unsigned for offset because it lets us simplify some math without > having to worry about signed overflow). > > Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v3: new patch > --- > include/block/nbd.h | 4 ++-- > nbd/server.c | 14 +++++++------- > qemu-nbd.c | 26 ++++++++++---------------- > 3 files changed, 19 insertions(+), 25 deletions(-) > > diff --git a/include/block/nbd.h b/include/block/nbd.h > index 1971b557896..0f252829376 100644 > --- a/include/block/nbd.h > +++ b/include/block/nbd.h > @@ -294,8 +294,8 @@ int nbd_errno_to_system_errno(int err); > typedef struct NBDExport NBDExport; > typedef struct NBDClient NBDClient; > > -NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size, > - const char *name, const char *description, > +NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset, > + int64_t size, const char *name, const char *desc, in previous patch you drop use of negative @size parameter, so it looks better to use unsigned for size like for offset > const char *bitmap, uint16_t nbdflags, > void (*close)(NBDExport *), bool writethrough, > BlockBackend *on_eject_blk, Error **errp); > diff --git a/nbd/server.c b/nbd/server.c > index c9937ccdc2a..15357d40fd7 100644 > --- a/nbd/server.c > +++ b/nbd/server.c > @@ -77,8 +77,8 @@ struct NBDExport { > BlockBackend *blk; > char *name; > char *description; > - off_t dev_offset; > - off_t size; > + uint64_t dev_offset; > + int64_t size; > uint16_t nbdflags; > QTAILQ_HEAD(, NBDClient) clients; > QTAILQ_ENTRY(NBDExport) next; > @@ -1455,8 +1455,8 @@ static void nbd_eject_notifier(Notifier *n, void *data) > nbd_export_close(exp); > } > > -NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size, > - const char *name, const char *description, > +NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset, > + int64_t size, const char *name, const char *desc, > const char *bitmap, uint16_t nbdflags, > void (*close)(NBDExport *), bool writethrough, > BlockBackend *on_eject_blk, Error **errp) > @@ -1497,7 +1497,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size, > exp->blk = blk; > exp->dev_offset = dev_offset; > exp->name = g_strdup(name); > - exp->description = g_strdup(description); > + exp->description = g_strdup(desc); unrelated but at least obvious, OK. However tiny note in commit message won't hurt. > exp->nbdflags = nbdflags; > assert(dev_offset <= size); > exp->size = QEMU_ALIGN_DOWN(size, BDRV_SECTOR_SIZE); > @@ -2131,8 +2131,8 @@ static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request, > if (request->from > client->exp->size || > request->from + request->len > client->exp->size) { > error_setg(errp, "operation past EOF; From: %" PRIu64 ", Len: %" PRIu32 > - ", Size: %" PRIu64, request->from, request->len, > - (uint64_t)client->exp->size); > + ", Size: %" PRId64, request->from, request->len, > + client->exp->size); > return (request->type == NBD_CMD_WRITE || > request->type == NBD_CMD_WRITE_ZEROES) ? -ENOSPC : -EINVAL; > } > diff --git a/qemu-nbd.c b/qemu-nbd.c > index ff4adb9b3eb..96c0829970c 100644 > --- a/qemu-nbd.c > +++ b/qemu-nbd.c > @@ -176,7 +176,7 @@ static void read_partition(uint8_t *p, struct partition_record *r) > } > > static int find_partition(BlockBackend *blk, int partition, > - off_t *offset, off_t *size) > + uint64_t *offset, int64_t *size) function never return negative @size, so what is the reason to keep it signed? Also, type conversion (uint64_t) should be dropped from the function code I think. > { > struct partition_record mbr[4]; > uint8_t data[MBR_SIZE]; > @@ -500,14 +500,14 @@ int main(int argc, char **argv) > { > BlockBackend *blk; > BlockDriverState *bs; > - off_t dev_offset = 0; > + uint64_t dev_offset = 0; > uint16_t nbdflags = 0; > bool disconnect = false; > const char *bindto = NULL; > const char *port = NULL; > char *sockpath = NULL; > char *device = NULL; > - off_t fd_size; > + int64_t fd_size; and here signed type is reasonable > QemuOpts *sn_opts = NULL; > const char *sn_id_or_name = NULL; > const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:x:T:D:B:"; > @@ -665,10 +665,6 @@ int main(int argc, char **argv) > error_report("Invalid offset `%s'", optarg); > exit(EXIT_FAILURE); > } > - if (dev_offset < 0) { > - error_report("Offset must be positive `%s'", optarg); > - exit(EXIT_FAILURE); > - } hm, then, may be, s/strtoll/strtoull before this? > break; > case 'l': > if (strstart(optarg, SNAPSHOT_OPT_BASE, NULL)) { > @@ -1005,15 +1001,14 @@ int main(int argc, char **argv) > } > > if (dev_offset >= fd_size) { > - error_report("Offset (%lld) has to be smaller than the image size " > - "(%lld)", > - (long long int)dev_offset, (long long int)fd_size); > + error_report("Offset (%" PRIu64 ") has to be smaller than the image " > + "size (%" PRIu64 ")", dev_offset, fd_size); PRId64 for fd_size > exit(EXIT_FAILURE); > } > fd_size -= dev_offset; > > if (partition != -1) { > - off_t limit; > + int64_t limit; > > if (dev_offset) { > error_report("Cannot request partition and offset together"); > @@ -1026,12 +1021,11 @@ int main(int argc, char **argv) > exit(EXIT_FAILURE); > } > /* partition limits are (32-bit << 9); can't overflow 64 bits */ > - assert(dev_offset >= 0 && dev_offset + limit >= dev_offset); > + assert(dev_offset + limit >= dev_offset); > if (dev_offset + limit > fd_size) { > - error_report("Discovered partition %d at offset %lld size %lld, " > - "but size exceeds file length %lld", partition, > - (long long int) dev_offset, (long long int) limit, > - (long long int) fd_size); > + error_report("Discovered partition %d at offset %" PRIu64 > + " size %" PRId64 ", but size exceeds file length %" > + PRId64, partition, dev_offset, limit, fd_size); > exit(EXIT_FAILURE); hmm, it may be better to place this patch before [03], to squash this chunk into [03] > } > fd_size = limit; >
On 1/15/19 4:19 AM, Vladimir Sementsov-Ogievskiy wrote: > 12.01.2019 20:57, Eric Blake wrote: >> Although our compile-time environment is set up so that we always >> support long files with 64-bit off_t, we have no guarantee whether >> off_t is the same type as int64_t. This requires casts when >> printing values, and prevents us from directly using qemu_strtoi64(). >> Let's just flip to [u]int64_t (signed for length, because we have to >> detect failure of blk_getlength() > > we have not, after previous patch nbd/server.c no longer has to check for blk_getlength() failures, but blockdev-nbd.c and qemu-nbd.c still do. Since the callers have to use an int64_t type for the length as part of their error checking, it's easier to accept an int64_t length to nbd_export_new(), even if nbd_export_new() could also use an unsigned type. > > and because off_t was signed; >> unsigned for offset because it lets us simplify some math without >> having to worry about signed overflow). >> >> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> >> --- >> v3: new patch >> --- >> include/block/nbd.h | 4 ++-- >> nbd/server.c | 14 +++++++------- >> qemu-nbd.c | 26 ++++++++++---------------- >> 3 files changed, 19 insertions(+), 25 deletions(-) >> >> diff --git a/include/block/nbd.h b/include/block/nbd.h >> index 1971b557896..0f252829376 100644 >> --- a/include/block/nbd.h >> +++ b/include/block/nbd.h >> @@ -294,8 +294,8 @@ int nbd_errno_to_system_errno(int err); >> typedef struct NBDExport NBDExport; >> typedef struct NBDClient NBDClient; >> >> -NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size, >> - const char *name, const char *description, >> +NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset, >> + int64_t size, const char *name, const char *desc, > > in previous patch you drop use of negative @size parameter, so it looks better > to use unsigned for size like for offset You can't have a size larger than 2^63; but an unsigned type holds nearly 2^64. I prefer a signed size, for the same reason that off_t is signed, in that checking for a negative size is easier to rule out sizes that are too large. >> >> static int find_partition(BlockBackend *blk, int partition, >> - off_t *offset, off_t *size) >> + uint64_t *offset, int64_t *size) > > function never return negative @size, so what is the reason to keep it signed? Because the C compiler does NOT like: int64_t len; find_partition(..., &len); with a uint64_t* parameter type - you HAVE to match the signed-ness of your caller's parameter with your pointer type. Since the caller already has to use a signed type (to check for blk_getlength() failure AND because sizes really cannot exceed 2^63), it's easier to keep it signed here. > > Also, type conversion (uint64_t) should be dropped from the function code I think. Are you talking about this part: if ((ext_partnum + j + 1) == partition) { *offset = (uint64_t)ext[j].start_sector_abs << 9; *size = (uint64_t)ext[j].nb_sectors_abs << 9; return 0; } } ext_partnum += 4; } else if ((i + 1) == partition) { *offset = (uint64_t)mbr[i].start_sector_abs << 9; *size = (uint64_t)mbr[i].nb_sectors_abs << 9; return 0; No - that has to keep the cast, because .start_sector_abs and .nb_sectors_abs are uint32_t values, but we want to shift into 64-bit results. You need the cast to force the correct arithmetic rather than truncating into a 32-bit value that then gets widened into 64-bit storage. >> @@ -665,10 +665,6 @@ int main(int argc, char **argv) >> error_report("Invalid offset `%s'", optarg); >> exit(EXIT_FAILURE); >> } >> - if (dev_offset < 0) { >> - error_report("Offset must be positive `%s'", optarg); >> - exit(EXIT_FAILURE); >> - } > > hm, then, may be, s/strtoll/strtoull before this? I clean that up in patch 6/19. > >> break; >> case 'l': >> if (strstart(optarg, SNAPSHOT_OPT_BASE, NULL)) { >> @@ -1005,15 +1001,14 @@ int main(int argc, char **argv) >> } >> >> if (dev_offset >= fd_size) { >> - error_report("Offset (%lld) has to be smaller than the image size " >> - "(%lld)", >> - (long long int)dev_offset, (long long int)fd_size); >> + error_report("Offset (%" PRIu64 ") has to be smaller than the image " >> + "size (%" PRIu64 ")", dev_offset, fd_size); > > PRId64 for fd_size Sure. >> @@ -1026,12 +1021,11 @@ int main(int argc, char **argv) >> exit(EXIT_FAILURE); >> } >> /* partition limits are (32-bit << 9); can't overflow 64 bits */ >> - assert(dev_offset >= 0 && dev_offset + limit >= dev_offset); >> + assert(dev_offset + limit >= dev_offset); >> if (dev_offset + limit > fd_size) { >> - error_report("Discovered partition %d at offset %lld size %lld, " >> - "but size exceeds file length %lld", partition, >> - (long long int) dev_offset, (long long int) limit, >> - (long long int) fd_size); >> + error_report("Discovered partition %d at offset %" PRIu64 >> + " size %" PRId64 ", but size exceeds file length %" >> + PRId64, partition, dev_offset, limit, fd_size); >> exit(EXIT_FAILURE); > > hmm, it may be better to place this patch before [03], to squash this chunk into [03] I didn't mind the churn; also, I prefer patch 3 first, because it's more likely to get backported as a bug fix than the rest of the series (and the earlier you stick backport candidates in a series, the easier it is to backport).
15.01.2019 18:33, Eric Blake wrote: > On 1/15/19 4:19 AM, Vladimir Sementsov-Ogievskiy wrote: >> 12.01.2019 20:57, Eric Blake wrote: >>> Although our compile-time environment is set up so that we always >>> support long files with 64-bit off_t, we have no guarantee whether >>> off_t is the same type as int64_t. This requires casts when >>> printing values, and prevents us from directly using qemu_strtoi64(). >>> Let's just flip to [u]int64_t (signed for length, because we have to >>> detect failure of blk_getlength() >> >> we have not, after previous patch > > nbd/server.c no longer has to check for blk_getlength() failures, but > blockdev-nbd.c and qemu-nbd.c still do. Since the callers have to use > an int64_t type for the length as part of their error checking, it's > easier to accept an int64_t length to nbd_export_new(), even if > nbd_export_new() could also use an unsigned type. > >> >> and because off_t was signed; >>> unsigned for offset because it lets us simplify some math without >>> having to worry about signed overflow). >>> >>> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>> Signed-off-by: Eric Blake <eblake@redhat.com> >>> >>> --- >>> v3: new patch >>> --- >>> include/block/nbd.h | 4 ++-- >>> nbd/server.c | 14 +++++++------- >>> qemu-nbd.c | 26 ++++++++++---------------- >>> 3 files changed, 19 insertions(+), 25 deletions(-) >>> >>> diff --git a/include/block/nbd.h b/include/block/nbd.h >>> index 1971b557896..0f252829376 100644 >>> --- a/include/block/nbd.h >>> +++ b/include/block/nbd.h >>> @@ -294,8 +294,8 @@ int nbd_errno_to_system_errno(int err); >>> typedef struct NBDExport NBDExport; >>> typedef struct NBDClient NBDClient; >>> >>> -NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size, >>> - const char *name, const char *description, >>> +NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset, >>> + int64_t size, const char *name, const char *desc, >> >> in previous patch you drop use of negative @size parameter, so it looks better >> to use unsigned for size like for offset > > You can't have a size larger than 2^63; but an unsigned type holds > nearly 2^64. I prefer a signed size, for the same reason that off_t is > signed, in that checking for a negative size is easier to rule out sizes > that are too large. > > >>> >>> static int find_partition(BlockBackend *blk, int partition, >>> - off_t *offset, off_t *size) >>> + uint64_t *offset, int64_t *size) >> >> function never return negative @size, so what is the reason to keep it signed? > > Because the C compiler does NOT like: > > int64_t len; > find_partition(..., &len); > > with a uint64_t* parameter type - you HAVE to match the signed-ness of > your caller's parameter with your pointer type. Since the caller already > has to use a signed type (to check for blk_getlength() failure AND > because sizes really cannot exceed 2^63), it's easier to keep it signed > here. > >> >> Also, type conversion (uint64_t) should be dropped from the function code I think. > > Are you talking about this part: > > if ((ext_partnum + j + 1) == partition) { > *offset = (uint64_t)ext[j].start_sector_abs << 9; > *size = (uint64_t)ext[j].nb_sectors_abs << 9; > return 0; > } > } > ext_partnum += 4; > } else if ((i + 1) == partition) { > *offset = (uint64_t)mbr[i].start_sector_abs << 9; > *size = (uint64_t)mbr[i].nb_sectors_abs << 9; > return 0; > > No - that has to keep the cast, because .start_sector_abs and > .nb_sectors_abs are uint32_t values, but we want to shift into 64-bit > results. You need the cast to force the correct arithmetic rather than > truncating into a 32-bit value that then gets widened into 64-bit storage. Oops, I'm stupid) I thought about something like (uint64_t)<variable that was off_t, but now it is uint64_t>, but pointed to <variable that was off_t, but now it is uint64_t> = (uint64_t)<something other> > >>> @@ -665,10 +665,6 @@ int main(int argc, char **argv) >>> error_report("Invalid offset `%s'", optarg); >>> exit(EXIT_FAILURE); >>> } >>> - if (dev_offset < 0) { >>> - error_report("Offset must be positive `%s'", optarg); >>> - exit(EXIT_FAILURE); >>> - } >> >> hm, then, may be, s/strtoll/strtoull before this? > > I clean that up in patch 6/19. > >> >>> break; >>> case 'l': >>> if (strstart(optarg, SNAPSHOT_OPT_BASE, NULL)) { >>> @@ -1005,15 +1001,14 @@ int main(int argc, char **argv) >>> } >>> >>> if (dev_offset >= fd_size) { >>> - error_report("Offset (%lld) has to be smaller than the image size " >>> - "(%lld)", >>> - (long long int)dev_offset, (long long int)fd_size); >>> + error_report("Offset (%" PRIu64 ") has to be smaller than the image " >>> + "size (%" PRIu64 ")", dev_offset, fd_size); >> >> PRId64 for fd_size > > Sure. > > >>> @@ -1026,12 +1021,11 @@ int main(int argc, char **argv) >>> exit(EXIT_FAILURE); >>> } >>> /* partition limits are (32-bit << 9); can't overflow 64 bits */ >>> - assert(dev_offset >= 0 && dev_offset + limit >= dev_offset); >>> + assert(dev_offset + limit >= dev_offset); >>> if (dev_offset + limit > fd_size) { >>> - error_report("Discovered partition %d at offset %lld size %lld, " >>> - "but size exceeds file length %lld", partition, >>> - (long long int) dev_offset, (long long int) limit, >>> - (long long int) fd_size); >>> + error_report("Discovered partition %d at offset %" PRIu64 >>> + " size %" PRId64 ", but size exceeds file length %" >>> + PRId64, partition, dev_offset, limit, fd_size); >>> exit(EXIT_FAILURE); >> >> hmm, it may be better to place this patch before [03], to squash this chunk into [03] > > I didn't mind the churn; also, I prefer patch 3 first, because it's more > likely to get backported as a bug fix than the rest of the series (and > the earlier you stick backport candidates in a series, the easier it is > to backport). >
15.01.2019 18:33, Eric Blake wrote: > On 1/15/19 4:19 AM, Vladimir Sementsov-Ogievskiy wrote: >> 12.01.2019 20:57, Eric Blake wrote: >>> Although our compile-time environment is set up so that we always >>> support long files with 64-bit off_t, we have no guarantee whether >>> off_t is the same type as int64_t. This requires casts when >>> printing values, and prevents us from directly using qemu_strtoi64(). >>> Let's just flip to [u]int64_t (signed for length, because we have to >>> detect failure of blk_getlength() >> >> we have not, after previous patch > > nbd/server.c no longer has to check for blk_getlength() failures, but > blockdev-nbd.c and qemu-nbd.c still do. Since the callers have to use > an int64_t type for the length as part of their error checking, it's > easier to accept an int64_t length to nbd_export_new(), even if > nbd_export_new() could also use an unsigned type. > >> >> and because off_t was signed; >>> unsigned for offset because it lets us simplify some math without >>> having to worry about signed overflow). >>> >>> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>> Signed-off-by: Eric Blake <eblake@redhat.com> >>> >>> --- >>> v3: new patch >>> --- >>> include/block/nbd.h | 4 ++-- >>> nbd/server.c | 14 +++++++------- >>> qemu-nbd.c | 26 ++++++++++---------------- >>> 3 files changed, 19 insertions(+), 25 deletions(-) >>> >>> diff --git a/include/block/nbd.h b/include/block/nbd.h >>> index 1971b557896..0f252829376 100644 >>> --- a/include/block/nbd.h >>> +++ b/include/block/nbd.h >>> @@ -294,8 +294,8 @@ int nbd_errno_to_system_errno(int err); >>> typedef struct NBDExport NBDExport; >>> typedef struct NBDClient NBDClient; >>> >>> -NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size, >>> - const char *name, const char *description, >>> +NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset, >>> + int64_t size, const char *name, const char *desc, >> >> in previous patch you drop use of negative @size parameter, so it looks better >> to use unsigned for size like for offset > > You can't have a size larger than 2^63; but an unsigned type holds > nearly 2^64. I prefer a signed size, for the same reason that off_t is > signed, in that checking for a negative size is easier to rule out sizes > that are too large. > > >>> >>> static int find_partition(BlockBackend *blk, int partition, >>> - off_t *offset, off_t *size) >>> + uint64_t *offset, int64_t *size) >> >> function never return negative @size, so what is the reason to keep it signed? > > Because the C compiler does NOT like: > > int64_t len; > find_partition(..., &len); > > with a uint64_t* parameter type - you HAVE to match the signed-ness of > your caller's parameter with your pointer type. Since the caller already > has to use a signed type (to check for blk_getlength() failure AND > because sizes really cannot exceed 2^63), it's easier to keep it signed > here. > >> >> Also, type conversion (uint64_t) should be dropped from the function code I think. > > Are you talking about this part: > > if ((ext_partnum + j + 1) == partition) { > *offset = (uint64_t)ext[j].start_sector_abs << 9; > *size = (uint64_t)ext[j].nb_sectors_abs << 9; > return 0; > } > } > ext_partnum += 4; > } else if ((i + 1) == partition) { > *offset = (uint64_t)mbr[i].start_sector_abs << 9; > *size = (uint64_t)mbr[i].nb_sectors_abs << 9; > return 0; > > No - that has to keep the cast, because .start_sector_abs and > .nb_sectors_abs are uint32_t values, but we want to shift into 64-bit > results. You need the cast to force the correct arithmetic rather than > truncating into a 32-bit value that then gets widened into 64-bit storage. > >>> @@ -665,10 +665,6 @@ int main(int argc, char **argv) >>> error_report("Invalid offset `%s'", optarg); >>> exit(EXIT_FAILURE); >>> } >>> - if (dev_offset < 0) { >>> - error_report("Offset must be positive `%s'", optarg); >>> - exit(EXIT_FAILURE); >>> - } >> >> hm, then, may be, s/strtoll/strtoull before this? > > I clean that up in patch 6/19. > >> >>> break; >>> case 'l': >>> if (strstart(optarg, SNAPSHOT_OPT_BASE, NULL)) { >>> @@ -1005,15 +1001,14 @@ int main(int argc, char **argv) >>> } >>> >>> if (dev_offset >= fd_size) { >>> - error_report("Offset (%lld) has to be smaller than the image size " >>> - "(%lld)", >>> - (long long int)dev_offset, (long long int)fd_size); >>> + error_report("Offset (%" PRIu64 ") has to be smaller than the image " >>> + "size (%" PRIu64 ")", dev_offset, fd_size); >> >> PRId64 for fd_size > > Sure. With this one fixed: Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Interesting, decided to search a bit, about don't we have an overflow, when adding request.offset to exp.dev_offset and found in nbd_co_receive_request: if (request->from > client->exp->size || request->from + request->len > client->exp->size) { shouldn't it be if (request->from > client->exp->size || request->len > client->exp->size - request->from) { to avoid overflow? > > >>> @@ -1026,12 +1021,11 @@ int main(int argc, char **argv) >>> exit(EXIT_FAILURE); >>> } >>> /* partition limits are (32-bit << 9); can't overflow 64 bits */ >>> - assert(dev_offset >= 0 && dev_offset + limit >= dev_offset); >>> + assert(dev_offset + limit >= dev_offset); >>> if (dev_offset + limit > fd_size) { >>> - error_report("Discovered partition %d at offset %lld size %lld, " >>> - "but size exceeds file length %lld", partition, >>> - (long long int) dev_offset, (long long int) limit, >>> - (long long int) fd_size); >>> + error_report("Discovered partition %d at offset %" PRIu64 >>> + " size %" PRId64 ", but size exceeds file length %" >>> + PRId64, partition, dev_offset, limit, fd_size); >>> exit(EXIT_FAILURE); >> >> hmm, it may be better to place this patch before [03], to squash this chunk into [03] > > I didn't mind the churn; also, I prefer patch 3 first, because it's more > likely to get backported as a bug fix than the rest of the series (and > the earlier you stick backport candidates in a series, the easier it is > to backport). >
On 1/16/19 2:23 AM, Vladimir Sementsov-Ogievskiy wrote: > > Interesting, decided to search a bit, about don't we have an overflow, > when adding request.offset to exp.dev_offset and found in nbd_co_receive_request: > > if (request->from > client->exp->size || > request->from + request->len > client->exp->size) { uint64_t + uint32_t > uint64_t > > shouldn't it be > > if (request->from > client->exp->size || > request->len > client->exp->size - request->from) { > > to avoid overflow? You are correct that the canonical way to check for overflow is to compare against subtraction, rather than do addition before compare. But we got lucky: even though client->exp->size is uint64_t, in practice it can only represent at most off_t (remember, off_t is signed), so the first leg of the branch proves that request->from fits in 63 bits, and thus the second is performing 63-bit + 32-bit which can be at most 64 bits, and therefore does not overflow. Still, I might rewrite it.
diff --git a/include/block/nbd.h b/include/block/nbd.h index 1971b557896..0f252829376 100644 --- a/include/block/nbd.h +++ b/include/block/nbd.h @@ -294,8 +294,8 @@ int nbd_errno_to_system_errno(int err); typedef struct NBDExport NBDExport; typedef struct NBDClient NBDClient; -NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size, - const char *name, const char *description, +NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset, + int64_t size, const char *name, const char *desc, const char *bitmap, uint16_t nbdflags, void (*close)(NBDExport *), bool writethrough, BlockBackend *on_eject_blk, Error **errp); diff --git a/nbd/server.c b/nbd/server.c index c9937ccdc2a..15357d40fd7 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -77,8 +77,8 @@ struct NBDExport { BlockBackend *blk; char *name; char *description; - off_t dev_offset; - off_t size; + uint64_t dev_offset; + int64_t size; uint16_t nbdflags; QTAILQ_HEAD(, NBDClient) clients; QTAILQ_ENTRY(NBDExport) next; @@ -1455,8 +1455,8 @@ static void nbd_eject_notifier(Notifier *n, void *data) nbd_export_close(exp); } -NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size, - const char *name, const char *description, +NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset, + int64_t size, const char *name, const char *desc, const char *bitmap, uint16_t nbdflags, void (*close)(NBDExport *), bool writethrough, BlockBackend *on_eject_blk, Error **errp) @@ -1497,7 +1497,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size, exp->blk = blk; exp->dev_offset = dev_offset; exp->name = g_strdup(name); - exp->description = g_strdup(description); + exp->description = g_strdup(desc); exp->nbdflags = nbdflags; assert(dev_offset <= size); exp->size = QEMU_ALIGN_DOWN(size, BDRV_SECTOR_SIZE); @@ -2131,8 +2131,8 @@ static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request, if (request->from > client->exp->size || request->from + request->len > client->exp->size) { error_setg(errp, "operation past EOF; From: %" PRIu64 ", Len: %" PRIu32 - ", Size: %" PRIu64, request->from, request->len, - (uint64_t)client->exp->size); + ", Size: %" PRId64, request->from, request->len, + client->exp->size); return (request->type == NBD_CMD_WRITE || request->type == NBD_CMD_WRITE_ZEROES) ? -ENOSPC : -EINVAL; } diff --git a/qemu-nbd.c b/qemu-nbd.c index ff4adb9b3eb..96c0829970c 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -176,7 +176,7 @@ static void read_partition(uint8_t *p, struct partition_record *r) } static int find_partition(BlockBackend *blk, int partition, - off_t *offset, off_t *size) + uint64_t *offset, int64_t *size) { struct partition_record mbr[4]; uint8_t data[MBR_SIZE]; @@ -500,14 +500,14 @@ int main(int argc, char **argv) { BlockBackend *blk; BlockDriverState *bs; - off_t dev_offset = 0; + uint64_t dev_offset = 0; uint16_t nbdflags = 0; bool disconnect = false; const char *bindto = NULL; const char *port = NULL; char *sockpath = NULL; char *device = NULL; - off_t fd_size; + int64_t fd_size; QemuOpts *sn_opts = NULL; const char *sn_id_or_name = NULL; const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:x:T:D:B:"; @@ -665,10 +665,6 @@ int main(int argc, char **argv) error_report("Invalid offset `%s'", optarg); exit(EXIT_FAILURE); } - if (dev_offset < 0) { - error_report("Offset must be positive `%s'", optarg); - exit(EXIT_FAILURE); - } break; case 'l': if (strstart(optarg, SNAPSHOT_OPT_BASE, NULL)) { @@ -1005,15 +1001,14 @@ int main(int argc, char **argv) } if (dev_offset >= fd_size) { - error_report("Offset (%lld) has to be smaller than the image size " - "(%lld)", - (long long int)dev_offset, (long long int)fd_size); + error_report("Offset (%" PRIu64 ") has to be smaller than the image " + "size (%" PRIu64 ")", dev_offset, fd_size); exit(EXIT_FAILURE); } fd_size -= dev_offset; if (partition != -1) { - off_t limit; + int64_t limit; if (dev_offset) { error_report("Cannot request partition and offset together"); @@ -1026,12 +1021,11 @@ int main(int argc, char **argv) exit(EXIT_FAILURE); } /* partition limits are (32-bit << 9); can't overflow 64 bits */ - assert(dev_offset >= 0 && dev_offset + limit >= dev_offset); + assert(dev_offset + limit >= dev_offset); if (dev_offset + limit > fd_size) { - error_report("Discovered partition %d at offset %lld size %lld, " - "but size exceeds file length %lld", partition, - (long long int) dev_offset, (long long int) limit, - (long long int) fd_size); + error_report("Discovered partition %d at offset %" PRIu64 + " size %" PRId64 ", but size exceeds file length %" + PRId64, partition, dev_offset, limit, fd_size); exit(EXIT_FAILURE); } fd_size = limit;
Although our compile-time environment is set up so that we always support long files with 64-bit off_t, we have no guarantee whether off_t is the same type as int64_t. This requires casts when printing values, and prevents us from directly using qemu_strtoi64(). Let's just flip to [u]int64_t (signed for length, because we have to detect failure of blk_getlength() and because off_t was signed; unsigned for offset because it lets us simplify some math without having to worry about signed overflow). Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Signed-off-by: Eric Blake <eblake@redhat.com> --- v3: new patch --- include/block/nbd.h | 4 ++-- nbd/server.c | 14 +++++++------- qemu-nbd.c | 26 ++++++++++---------------- 3 files changed, 19 insertions(+), 25 deletions(-)