From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44868) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bXNi6-0003Qx-Jo for qemu-devel@nongnu.org; Wed, 10 Aug 2016 03:13:55 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bXNi3-0004P5-B3 for qemu-devel@nongnu.org; Wed, 10 Aug 2016 03:13:54 -0400 Received: from mx1.redhat.com ([209.132.183.28]:35540) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bXNi3-0004Oz-2u for qemu-devel@nongnu.org; Wed, 10 Aug 2016 03:13:51 -0400 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) (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 8F7DEC04B30A for ; Wed, 10 Aug 2016 07:13:50 +0000 (UTC) From: Markus Armbruster References: <1470732609-15488-1-git-send-email-prasanna.kalever@redhat.com> <20160810010546.GE5270@localhost.localdomain> Date: Wed, 10 Aug 2016 09:13:48 +0200 In-Reply-To: <20160810010546.GE5270@localhost.localdomain> (Jeff Cody's message of "Tue, 9 Aug 2016 21:05:46 -0400") Message-ID: <87y4453xlf.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain 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: Jeff Cody Cc: Prasanna Kumar Kalever , vbellur@redhat.com, areis@redhat.com, qemu-devel@nongnu.org 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