From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55202) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c0SdF-0000Qb-IM for qemu-devel@nongnu.org; Sat, 29 Oct 2016 08:21:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c0SdA-000830-Pt for qemu-devel@nongnu.org; Sat, 29 Oct 2016 08:21:05 -0400 Received: from mx1.redhat.com ([209.132.183.28]:52768) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1c0SdA-00082d-Au for qemu-devel@nongnu.org; Sat, 29 Oct 2016 08:21:00 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 7BAEC83F47 for ; Sat, 29 Oct 2016 12:20:59 +0000 (UTC) Date: Sat, 29 Oct 2016 08:20:57 -0400 From: Jeff Cody Message-ID: <20161029122056.GD17785@localhost.localdomain> References: <1470732609-15488-1-git-send-email-prasanna.kalever@redhat.com> <20160810010546.GE5270@localhost.localdomain> <87y4453xlf.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87y4453xlf.fsf@dusky.pond.sub.org> Subject: Re: [Qemu-devel] [PATCH v2 1/1] block/gluster: improve defense over string to int conversion List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: Prasanna Kumar Kalever , vbellur@redhat.com, qemu-devel@nongnu.org On Wed, Aug 10, 2016 at 09:13:48AM +0200, Markus Armbruster wrote: > Jeff Cody writes: > > > On Tue, Aug 09, 2016 at 02:20:09PM +0530, Prasanna Kumar Kalever wrote: > >> using atoi() for converting string to int may be error prone in case if > >> string supplied in the argument is not a fold of numerical number, > >> > >> This is not a bug because in the existing code, > >> > >> static QemuOptsList runtime_tcp_opts = { > >> .name = "gluster_tcp", > >> .head = QTAILQ_HEAD_INITIALIZER(runtime_tcp_opts.head), > >> .desc = { > >> ... > >> { > >> .name = GLUSTER_OPT_PORT, > >> .type = QEMU_OPT_NUMBER, > >> .help = "port number ...", > >> }, > >> ... > >> }; > >> > >> port type is QEMU_OPT_NUMBER, before we actually reaches atoi() port is already > >> defended by parse_option_number() > >> > >> However It is a good practice to use function like parse_uint_full() > >> over atoi() to keep port self defended > >> > >> Note: As now the port string to int conversion has its defence code set, > >> and also we understand that port argument is actually a string type, > >> in the follow up patch let's move port type from QEMU_OPT_NUMBER to > >> QEMU_OPT_STRING > >> > >> Signed-off-by: Prasanna Kumar Kalever > >> --- > >> v1: Initial patch > >> v2: Address comments on v1 given by Markus > >> --- > >> block/gluster.c | 11 ++++++++++- > >> 1 file changed, 10 insertions(+), 1 deletion(-) > >> > >> diff --git a/block/gluster.c b/block/gluster.c > >> index 01b479f..edde1ad 100644 > >> --- a/block/gluster.c > >> +++ b/block/gluster.c > >> @@ -14,6 +14,7 @@ > >> #include "qapi/qmp/qerror.h" > >> #include "qemu/uri.h" > >> #include "qemu/error-report.h" > >> +#include "qemu/cutils.h" > >> > >> #define GLUSTER_OPT_FILENAME "filename" > >> #define GLUSTER_OPT_VOLUME "volume" > >> @@ -318,6 +319,7 @@ static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf, > >> int ret; > >> int old_errno; > >> GlusterServerList *server; > >> + unsigned long long port; > >> > >> glfs = glfs_new(gconf->volume); > >> if (!glfs) { > >> @@ -330,10 +332,17 @@ static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf, > >> GlusterTransport_lookup[server->value->type], > >> server->value->u.q_unix.path, 0); > >> } else { > >> + if ((parse_uint_full(server->value->u.tcp.port, &port, 10) < 0) || > >> + (port > 65535)) { > > Two pairs of superfluous parenthesis. Better: > > if (parse_uint_full(server->value->u.tcp.port, &port, 10) < 0 || > port > 65535) { > > or, if you prefer to break before the operator (I do): > > if (parse_uint_full(server->value->u.tcp.port, &port, 10) < 0 > || port > 65535) { > > or, if you prefer to keep side effects out of complex conditionals: > > ret = parse_uint_full(server->value->u.tcp.port, &port, 10); > if (ret < 0 || port > 65535) { > > Maintainer's choice. Perhaps Jeff can touch this up on commit. > > >> + error_setg(errp, "'%s' is not a valid port number", > >> + server->value->u.tcp.port); > >> + errno = EINVAL; > > > > As long as we are range checking, we should probably kick back 0 as an > > invalid port number as well, right? > > Port 0 is an oddity. On the one hand, bind() interprets it as "pick an > ephemeral port for me". That makes port 0 unusable with the classical > sockets interface. On the other hand, IP doesn't treat port 0 > specially. You *can* send and receive port 0 packets with a raw socket > interface. > > Anyway, I wouldn't bother rejecting port 0 here. Just let the system do > its "pick an ephemeral port" magic. > > >> + goto out; > >> + } > >> ret = glfs_set_volfile_server(glfs, > >> GlusterTransport_lookup[server->value->type], > >> server->value->u.tcp.host, > >> - atoi(server->value->u.tcp.port)); > >> + (int)port); > >> } > >> > >> if (ret < 0) { > >> -- > >> 2.7.4 > >> > > Preferrably with the parenthesis cleaned up: > Reviewed-by: Markus Armbruster Thanks, Applied to my block branch: git://github.com/codyprime/qemu-kvm-jtc.git block [Removed spurious parenthesis] -Jeff