From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=60718 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1P5Hwx-0007xs-Gi for qemu-devel@nongnu.org; Mon, 11 Oct 2010 08:57:58 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1P5Hkx-0006Dn-Us for qemu-devel@nongnu.org; Mon, 11 Oct 2010 08:45:33 -0400 Received: from mx1.redhat.com ([209.132.183.28]:1780) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1P5Hkx-0006De-NJ for qemu-devel@nongnu.org; Mon, 11 Oct 2010 08:45:31 -0400 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o9BCjUnn006249 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 11 Oct 2010 08:45:30 -0400 Message-ID: <4CB306E8.5010400@redhat.com> Date: Mon, 11 Oct 2010 14:45:28 +0200 From: Jes Sorensen MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 1/7] Introduce strtosz() library function to convert a string to a byte count. References: <1286529360-5715-1-git-send-email-Jes.Sorensen@redhat.com> <1286529360-5715-2-git-send-email-Jes.Sorensen@redhat.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: pbonzini@redhat.com, qemu-devel@nongnu.org On 10/11/10 10:51, Markus Armbruster wrote: > Jes.Sorensen@redhat.com writes: >> +/* >> + * Convert string to bytes, allowing either K/k for KB, M/m for MB, >> + * G/g for GB or T/t for TB. Default without any postfix is MB. >> + * End pointer will be returned in *end, if end is valid. >> + * Return -1 on error. >> + */ >> +ssize_t strtosz(const char *nptr, char **end) >> +{ >> + int64_t value; > > long long, please, because that's what strtoll() returns. I merged patches 1-3 as you suggested, so comments based on the combined patch. This is longer an issue as it is switched to strtod(). >> + char *endptr; >> + >> + value = strtoll(nptr, &endptr, 0); >> + switch (*endptr++) { >> + case 'K': >> + case 'k': >> + value <<= 10; >> + break; >> + case 0: >> + case 'M': >> + case 'm': >> + value <<= 20; >> + break; >> + case 'G': >> + case 'g': >> + value <<= 30; >> + break; >> + case 'T': >> + case 't': >> + value <<= 40; >> + break; >> + default: >> + value = -1; >> + } >> + >> + if (end) >> + *end = endptr; >> + >> + return value; > > Casts value to ssize_t, which might truncate. The new patch does: int64_t tmpval; tmpval = (val * mul); if (tmpval >= ~(size_t)0) goto fail; so anything that is out of bounds is checked and caught before returning a possibly truncated valued. > Sloppy use of strtoll(). tmpval = (val * mul); if (tmpval >= ~(size_t)0) goto fail; > > Both tolerable as long as the patch doesn't make things worse. Let's > see: > >> diff --git a/qemu-common.h b/qemu-common.h >> index 81aafa0..0a062d4 100644 >> --- a/qemu-common.h >> +++ b/qemu-common.h >> @@ -153,6 +153,7 @@ time_t mktimegm(struct tm *tm); >> int qemu_fls(int i); >> int qemu_fdatasync(int fd); >> int fcntl_setfl(int fd, int flag); >> +ssize_t strtosz(const char *nptr, char **end); >> >> /* path.c */ >> void init_paths(const char *prefix); >> diff --git a/vl.c b/vl.c >> index df414ef..6043fa2 100644 >> --- a/vl.c >> +++ b/vl.c >> @@ -734,16 +734,13 @@ static void numa_add(const char *optarg) >> if (get_param_value(option, 128, "mem", optarg) == 0) { >> node_mem[nodenr] = 0; >> } else { >> - value = strtoull(option, &endptr, 0); >> - switch (*endptr) { >> - case 0: case 'M': case 'm': >> - value <<= 20; >> - break; >> - case 'G': case 'g': >> - value <<= 30; >> - break; >> + ssize_t sval; >> + sval = strtosz(option, NULL); >> + if (sval < 0) { >> + fprintf(stderr, "qemu: invalid numa mem size: %s\n", optarg); >> + exit(1); > > Before After > Invalid number silently interpreted as zero no change > Overflow silently capped to ULLONG_MAX LLONG_MAX, then > trunc ssize_t > Invalid size suffix silently ignored rejected What do you mean by invalid number here? LLONG_MAX seems a reasonable limit, ie. 31 bit on 32 bit hosts or 63 bits on 64 bit hosts. strtosz() is meant to return a size, so IMHO thats a fair limitation. We could use size_t instead of ssize_t but it would require ugly casts in any function calling the function to test for error. > Before After > Invalid number silently interpreted as zero no change > Overflow silently capped to ULLONG_MAX LLONG_MAX, then > trunc ssize_t > Invalid size suffix rejected no change > > A bit more context: > > > /* On 32-bit hosts, QEMU is limited by virtual address space */ > if (value > (2047 << 20) && HOST_LONG_BITS == 32) { > fprintf(stderr, "qemu: at most 2047 MB RAM can be simulated\n"); > exit(1); > } > if (value != (uint64_t)(ram_addr_t)value) { > fprintf(stderr, "qemu: ram size too large\n"); > exit(1); > } > ram_size = value; > break; > > I'm afraid you break both conditionals for 32 bit hosts. > > On such hosts, ssize_t is 32 bits wide. strtosz() parses 64 bits > internally, but truncates to 32 bits silently. I believe the combined patch is taking care of this fine with the >= ~(size_t)0 comparison. If the value is above that, it returns an error. For 32 bit hosts that means we should be able to specify 2047MB RAM fine. The only place where I see the latter being a potential problem is on P64 systems such as win64, since ram_addr_t is defined to be unsigned long, but afaik we don't support win64 anyway. On both 32 bit and LP64 systems sizeof(ram_addr_t) == sizeof(ssize_t), so it should be fine. > The old code reliably rejects values larger than 2047MiB. Your > truncation can change a value exceeding the limit to one within the > limit. First conditional becomes unreliable. > > The second conditional becomes useless: it sign-extends a non-negative > 32 bit integer value to 64 bit, then truncates back, and checks the > value stays the same. It trivially does. > > I strongly recommend to make strtosz() sane from the start, not in a > later patch: proper error checking, including proper handling of > overflow. > > Perhaps squashing 1-3/7 would get us there, or at least closer. I have squashed 1-3, but I don't think 7 should be squashed. Adding a new feature and taking away the old one in the same patch isn't right IMHO. Cheers, Jes