Re: [Qemu-devel] [PATCH 04/14] qemu-nbd: Simplify --partition handling
On 11/30/18 4:03 PM, Eric Blake wrote:
Our open-coding of strtol handling forgot to handle overflow
conditions. What's more, since we insiste on a user-supplied
partition to be non-zero, we can use 0 rather than -1 for our
initial value to distinguish when a partition is not being
served, for slightly more optimal code.
Signed-off-by: Eric Blake
---
qemu-nbd.c | 14 +-
1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 55e29bd9a7e..866e64779f1 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -546,7 +546,7 @@ int main(int argc, char **argv)
int opt_ind = 0;
char *end;
int flags = BDRV_O_RDWR;
-int partition = -1;
+int partition = 0;
int ret = 0;
bool seen_cache = false;
bool seen_discard = false;
@@ -685,13 +685,9 @@ int main(int argc, char **argv)
flags &= ~BDRV_O_RDWR;
break;
case 'P':
-partition = strtol(optarg, , 0);
-if (*end) {
-error_report("Invalid partition `%s'", optarg);
-exit(EXIT_FAILURE);
-}
-if (partition < 1 || partition > 8) {
-error_report("Invalid partition %d", partition);
+if (qemu_strtoi(optarg, NULL, 0, ) < 0 ||
Hmm - the fact that 'char *end' is not a dead variable means there are
more uses of strtoll() that need fixing. I'll get those in v2.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 04/14] qemu-nbd: Simplify --partition handling
On 12/5/18 10:26 AM, Eric Blake wrote: is it possible, that "char *ep" remains uninitialized, and than we access it in check_strtox_error? I don's see in strtol spec a guarantee of setting endptr on failure path. C99 7.10.1.4 P5-7 requires strtoll() and friends to assign through 'endptr' if it is non-NULL, for both success and ERANGE failure cases. POSIX then further requires 'endptr' to be set for EINVAL failures due to out-of-range 'base' (not that we have any such callers), and permits (but does not require) the empty string to cause an EINVAL failure (but whether or not EINVAL happened, 'endptr' is still set). There are no other possible failures, so no, we are not dereferencing an uninitialized variable in check_strtox_error. Correction, quoting POSIX: http://pubs.opengroup.org/onlinepubs/9699919799/functions/strtol.html APPLICATION USAGE Since the value of *endptr is unspecified if the value of base is not supported, applications should either ensure that base has a supported value (0 or between 2 and 36) before the call, or check for an [EINVAL] error before examining *endptr. So yes, we CAN end up transferring an uninitialized variable into the caller's non-NULL endpointer if the caller passes an out-of-range base (this particular caller passes NULL for an endpointer, and an in-range base, so it's not an issue). Might be worth a separate patch to assert that base is in range for all of the qemu_strto* helpers, if we are worried (I'm not). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 04/14] qemu-nbd: Simplify --partition handling
On 12/5/18 9:40 AM, Vladimir Sementsov-Ogievskiy wrote:
01.12.2018 1:03, Eric Blake wrote:
Our open-coding of strtol handling forgot to handle overflow
conditions. What's more, since we insiste on a user-supplied
partition to be non-zero, we can use 0 rather than -1 for our
initial value to distinguish when a partition is not being
served, for slightly more optimal code.
-if (partition < 1 || partition > 8) {
-error_report("Invalid partition %d", partition);
+if (qemu_strtoi(optarg, NULL, 0, ) < 0 ||
I decided to look into qemu_strtoi, hmm.
is it possible, that "char *ep" remains uninitialized, and than we access
it in check_strtox_error? I don's see in strtol spec a guarantee of setting
endptr on failure path.
C99 7.10.1.4 P5-7 requires strtoll() and friends to assign through
'endptr' if it is non-NULL, for both success and ERANGE failure cases.
POSIX then further requires 'endptr' to be set for EINVAL failures due
to out-of-range 'base' (not that we have any such callers), and permits
(but does not require) the empty string to cause an EINVAL failure (but
whether or not EINVAL happened, 'endptr' is still set). There are no
other possible failures, so no, we are not dereferencing an
uninitialized variable in check_strtox_error.
+partition < 1 || partition > 8) {
don't you like brace on separate line after multi-line conditions?
CODING_STYLE is silent on the matter; checkpatch.pl allows either style
for multi-line, while requesting the same line as the condition for
one-line (see commit a97ceca57). But since I'm already in the habit of
putting { right after the condition because of checkpatch, I don't go
out of my way to put it on its own line after multi-line conditionals.
I don't think it matters too much.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 04/14] qemu-nbd: Simplify --partition handling
01.12.2018 1:03, Eric Blake wrote:
> Our open-coding of strtol handling forgot to handle overflow
> conditions. What's more, since we insiste on a user-supplied
> partition to be non-zero, we can use 0 rather than -1 for our
> initial value to distinguish when a partition is not being
> served, for slightly more optimal code.
>
> Signed-off-by: Eric Blake
> ---
> qemu-nbd.c | 14 +-
> 1 file changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index 55e29bd9a7e..866e64779f1 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -546,7 +546,7 @@ int main(int argc, char **argv)
> int opt_ind = 0;
> char *end;
> int flags = BDRV_O_RDWR;
> -int partition = -1;
> +int partition = 0;
> int ret = 0;
> bool seen_cache = false;
> bool seen_discard = false;
> @@ -685,13 +685,9 @@ int main(int argc, char **argv)
> flags &= ~BDRV_O_RDWR;
> break;
> case 'P':
> -partition = strtol(optarg, , 0);
> -if (*end) {
> -error_report("Invalid partition `%s'", optarg);
> -exit(EXIT_FAILURE);
> -}
> -if (partition < 1 || partition > 8) {
> -error_report("Invalid partition %d", partition);
> +if (qemu_strtoi(optarg, NULL, 0, ) < 0 ||
I decided to look into qemu_strtoi, hmm.
is it possible, that "char *ep" remains uninitialized, and than we access
it in check_strtox_error? I don's see in strtol spec a guarantee of setting
endptr on failure path.
> +partition < 1 || partition > 8) {
don't you like brace on separate line after multi-line conditions?
> +error_report("Invalid partition %s", optarg);
> exit(EXIT_FAILURE);
> }
> break;
> @@ -1004,7 +1000,7 @@ int main(int argc, char **argv)
> }
> fd_size -= dev_offset;
>
> -if (partition != -1) {
> +if (partition) {
> ret = find_partition(blk, partition, _offset, _size);
> if (ret < 0) {
> error_report("Could not find partition %d: %s", partition,
>
Reviewed-by: Vladimir Sementsov-Ogievskiy
--
Best regards,
Vladimir
Re: [Qemu-devel] [PATCH 04/14] qemu-nbd: Simplify --partition handling
On 11/30/18 4:26 PM, Richard W.M. Jones wrote:
On Fri, Nov 30, 2018 at 04:03:33PM -0600, Eric Blake wrote:
Our open-coding of strtol handling forgot to handle overflow
conditions. What's more, since we insiste on a user-supplied
"insist"
(Ever wonder if I stick in a typo on purpose, just to see who reviews
closely?)
partition to be non-zero, we can use 0 rather than -1 for our
initial value to distinguish when a partition is not being
served, for slightly more optimal code.
Signed-off-by: Eric Blake
---
qemu-nbd.c | 14 +-
1 file changed, 5 insertions(+), 9 deletions(-)
+if (qemu_strtoi(optarg, NULL, 0, ) < 0 ||
+partition < 1 || partition > 8) {
+error_report("Invalid partition %s", optarg);
Pffft only supporting a mere 8 partitions :-? I raised the limits in
nbdkit recently so it can handle an infinite number of partitions :-)
nbdkit also handles GPT partitions. qemu-nbd is still stuck on MBR:
static int find_partition(BlockBackend *blk, int partition,
off_t *offset, off_t *size)
{
struct partition_record mbr[4];
uint8_t data[MBR_SIZE];
and if I read git blame correctly, the partition code in qemu-nbd is
mostly untouched since Fabrice committed Anthony's initial
implementation in 2008.
Also, reading the code, I see that qemu-nbd allows partitions 5-8
because it reads through the extended partition descriptor in 4 to
expose the logical partitions within; which nbdkit can't do yet :)
Anyway, it's a problem with the existing code, so doesn't affect the
review.
Indeed, and with nbdkit around, I'm wondering if I should deprecate
'qemu-nbd --partition' and just point people to nbdkit instead
(provided, of course, that nbdkit learns logical partitions).
Even if you have a qcow2 image with MBR partitions, and where we don't
want to write a qcow2 plugin in nbdkit, you can still always rewrite:
client -> qemu-nbd --partition=1
into
client -> nbdkit --filter=partition nbd partition=1 -> qemu-nbd
(although I don't know what the performance penalty would be)
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 04/14] qemu-nbd: Simplify --partition handling
On Fri, Nov 30, 2018 at 04:03:33PM -0600, Eric Blake wrote:
> Our open-coding of strtol handling forgot to handle overflow
> conditions. What's more, since we insiste on a user-supplied
"insist"
> partition to be non-zero, we can use 0 rather than -1 for our
> initial value to distinguish when a partition is not being
> served, for slightly more optimal code.
>
> Signed-off-by: Eric Blake
> ---
> qemu-nbd.c | 14 +-
> 1 file changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index 55e29bd9a7e..866e64779f1 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -546,7 +546,7 @@ int main(int argc, char **argv)
> int opt_ind = 0;
> char *end;
> int flags = BDRV_O_RDWR;
> -int partition = -1;
> +int partition = 0;
> int ret = 0;
> bool seen_cache = false;
> bool seen_discard = false;
> @@ -685,13 +685,9 @@ int main(int argc, char **argv)
> flags &= ~BDRV_O_RDWR;
> break;
> case 'P':
> -partition = strtol(optarg, , 0);
> -if (*end) {
> -error_report("Invalid partition `%s'", optarg);
> -exit(EXIT_FAILURE);
> -}
> -if (partition < 1 || partition > 8) {
> -error_report("Invalid partition %d", partition);
> +if (qemu_strtoi(optarg, NULL, 0, ) < 0 ||
> +partition < 1 || partition > 8) {
> +error_report("Invalid partition %s", optarg);
Pffft only supporting a mere 8 partitions :-? I raised the limits in
nbdkit recently so it can handle an infinite number of partitions :-)
Anyway, it's a problem with the existing code, so doesn't affect the
review.
> exit(EXIT_FAILURE);
> }
> break;
> @@ -1004,7 +1000,7 @@ int main(int argc, char **argv)
> }
> fd_size -= dev_offset;
>
> -if (partition != -1) {
> +if (partition) {
> ret = find_partition(blk, partition, _offset, _size);
> if (ret < 0) {
> error_report("Could not find partition %d: %s", partition,
> --
> 2.17.2
Reviewed-by: Richard W.M. Jones
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html
[Qemu-devel] [PATCH 04/14] qemu-nbd: Simplify --partition handling
Our open-coding of strtol handling forgot to handle overflow
conditions. What's more, since we insiste on a user-supplied
partition to be non-zero, we can use 0 rather than -1 for our
initial value to distinguish when a partition is not being
served, for slightly more optimal code.
Signed-off-by: Eric Blake
---
qemu-nbd.c | 14 +-
1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 55e29bd9a7e..866e64779f1 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -546,7 +546,7 @@ int main(int argc, char **argv)
int opt_ind = 0;
char *end;
int flags = BDRV_O_RDWR;
-int partition = -1;
+int partition = 0;
int ret = 0;
bool seen_cache = false;
bool seen_discard = false;
@@ -685,13 +685,9 @@ int main(int argc, char **argv)
flags &= ~BDRV_O_RDWR;
break;
case 'P':
-partition = strtol(optarg, , 0);
-if (*end) {
-error_report("Invalid partition `%s'", optarg);
-exit(EXIT_FAILURE);
-}
-if (partition < 1 || partition > 8) {
-error_report("Invalid partition %d", partition);
+if (qemu_strtoi(optarg, NULL, 0, ) < 0 ||
+partition < 1 || partition > 8) {
+error_report("Invalid partition %s", optarg);
exit(EXIT_FAILURE);
}
break;
@@ -1004,7 +1000,7 @@ int main(int argc, char **argv)
}
fd_size -= dev_offset;
-if (partition != -1) {
+if (partition) {
ret = find_partition(blk, partition, _offset, _size);
if (ret < 0) {
error_report("Could not find partition %d: %s", partition,
--
2.17.2
