From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49107) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YVeqP-00021a-9X for qemu-devel@nongnu.org; Wed, 11 Mar 2015 07:30:34 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YVeqM-0000Nj-3O for qemu-devel@nongnu.org; Wed, 11 Mar 2015 07:30:33 -0400 Sender: Paolo Bonzini Message-ID: <55002750.6000008@redhat.com> Date: Wed, 11 Mar 2015 12:30:24 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <1424887718-10800-1-git-send-email-mreitz@redhat.com> <1424887718-10800-12-git-send-email-mreitz@redhat.com> In-Reply-To: <1424887718-10800-12-git-send-email-mreitz@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 11/25] qemu-nbd: Fix and improve input verification List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz , qemu-block@nongnu.org Cc: Kevin Wolf , qemu-devel@nongnu.org, Stefan Hajnoczi On 25/02/2015 19:08, Max Reitz wrote: > This patch makes sure the result of strtol() does not overflow (by > storing it in long integers instead of plain integers, and by checking > errno), allows the user to specify "--discard on" and > "--detect-zeroes unmap" in any order and strips the trailing \n from two > error messages. > > Signed-off-by: Max Reitz > --- > qemu-nbd.c | 40 +++++++++++++++++++++++++++------------- > 1 file changed, 27 insertions(+), 13 deletions(-) Let's introduce a general purpose utility instead of duplicating the checks in every strto* caller. For example, you can ensure that errno is checked and, if NULL is passed as the second argument, that the whole string is a number. Something like this: int qemu_strtol(const char *name, const char **next, int base, long *result) { char *p; errno = 0; *result = strtol(name, &p, base); if (!next && *p) { return -EINVAL; } if (next) { *next = p; } return -errno; } Paolo > diff --git a/qemu-nbd.c b/qemu-nbd.c > index fd1e0c8..7376a35 100644 > --- a/qemu-nbd.c > +++ b/qemu-nbd.c > @@ -51,7 +51,7 @@ static char *srcpath; > static char *sockpath; > static int persistent = 0; > static enum { RUNNING, TERMINATE, TERMINATING, TERMINATED } state; > -static int shared = 1; > +static long shared = 1; > static int nb_fds; > > static void usage(const char *name) > @@ -432,10 +432,10 @@ int main(int argc, char **argv) > }; > int ch; > int opt_ind = 0; > - int li; > + long li; > char *end; > int flags = BDRV_O_RDWR; > - int partition = -1; > + long partition = -1; > int ret = 0; > int fd; > bool seen_cache = false; > @@ -510,11 +510,6 @@ int main(int argc, char **argv) > errx(EXIT_FAILURE, "Failed to parse detect_zeroes mode: %s", > error_get_pretty(local_err)); > } > - if (detect_zeroes == BLOCKDEV_DETECT_ZEROES_OPTIONS_UNMAP && > - !(flags & BDRV_O_UNMAP)) { > - errx(EXIT_FAILURE, "setting detect-zeroes to unmap is not allowed " > - "without setting discard operation to unmap"); > - } > break; > case 'b': > bindto = optarg; > @@ -530,13 +525,17 @@ int main(int argc, char **argv) > port = (uint16_t)li; > break; > case 'o': > - dev_offset = strtoll (optarg, &end, 0); > + errno = 0; > + dev_offset = strtoll(optarg, &end, 0); > if (*end) { > errx(EXIT_FAILURE, "Invalid offset `%s'", optarg); > } > if (dev_offset < 0) { > errx(EXIT_FAILURE, "Offset must be positive `%s'", optarg); > } > + if (errno) { > + err(EXIT_FAILURE, "Invalid offset `%s'", optarg); > + } > break; > case 'l': > if (strstart(optarg, SNAPSHOT_OPT_BASE, NULL)) { > @@ -559,13 +558,13 @@ int main(int argc, char **argv) > errx(EXIT_FAILURE, "Invalid partition `%s'", optarg); > } > if (partition < 1 || partition > 8) { > - errx(EXIT_FAILURE, "Invalid partition %d", partition); > + errx(EXIT_FAILURE, "Invalid partition %s", optarg); > } > break; > case 'k': > sockpath = optarg; > if (sockpath[0] != '/') { > - errx(EXIT_FAILURE, "socket path must be absolute\n"); > + errx(EXIT_FAILURE, "socket path must be absolute"); > } > break; > case 'd': > @@ -580,7 +579,12 @@ int main(int argc, char **argv) > errx(EXIT_FAILURE, "Invalid shared device number '%s'", optarg); > } > if (shared < 1) { > - errx(EXIT_FAILURE, "Shared device number must be greater than 0\n"); > + errx(EXIT_FAILURE, > + "Shared device number must be greater than 0"); > + } > + if (shared >= INT_MAX) { > + errx(EXIT_FAILURE, > + "Shared device number must be less than %i", INT_MAX); > } > break; > case 'f': > @@ -606,6 +610,12 @@ int main(int argc, char **argv) > } > } > > + if (detect_zeroes == BLOCKDEV_DETECT_ZEROES_OPTIONS_UNMAP && > + !(flags & BDRV_O_UNMAP)) { > + errx(EXIT_FAILURE, "Setting detect-zeroes to unmap is not allowed " > + "without setting discard operation to unmap"); > + } > + > if ((argc - optind) != 1) { > errx(EXIT_FAILURE, "Invalid number of argument.\n" > "Try `%s --help' for more information.", > @@ -730,10 +740,14 @@ int main(int argc, char **argv) > } > > if (partition != -1) { > + if (dev_offset) { > + errx(EXIT_FAILURE, "Cannot use both -o and -P at the same time"); > + } > + > ret = find_partition(blk, partition, &dev_offset, &fd_size); > if (ret < 0) { > errno = -ret; > - err(EXIT_FAILURE, "Could not find partition %d", partition); > + err(EXIT_FAILURE, "Could not find partition %ld", partition); > } > } > >