From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59697) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YXVVm-0003Z2-C8 for qemu-devel@nongnu.org; Mon, 16 Mar 2015 09:56:55 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YXVVh-00069d-AV for qemu-devel@nongnu.org; Mon, 16 Mar 2015 09:56:54 -0400 Message-ID: <5506E113.50908@redhat.com> Date: Mon, 16 Mar 2015 09:56:35 -0400 From: Max Reitz MIME-Version: 1.0 References: <1424887718-10800-1-git-send-email-mreitz@redhat.com> <1424887718-10800-12-git-send-email-mreitz@redhat.com> <55002750.6000008@redhat.com> In-Reply-To: <55002750.6000008@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed 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: Paolo Bonzini , qemu-block@nongnu.org Cc: Kevin Wolf , qemu-devel@nongnu.org, Stefan Hajnoczi On 2015-03-11 at 07:30, Paolo Bonzini wrote: > > 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; > } I was afraid you might say this... Will do. Thank you for reviewing! Max > 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); >> } >> } >> >>