From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39015) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bXHy4-0002tD-9K for qemu-devel@nongnu.org; Tue, 09 Aug 2016 21:06:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bXHy0-0005dL-2K for qemu-devel@nongnu.org; Tue, 09 Aug 2016 21:05:59 -0400 Received: from mx1.redhat.com ([209.132.183.28]:56304) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bXHxw-0005b9-9z for qemu-devel@nongnu.org; Tue, 09 Aug 2016 21:05:55 -0400 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (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 3DB8061E4C for ; Wed, 10 Aug 2016 01:05:51 +0000 (UTC) Date: Tue, 9 Aug 2016 21:05:46 -0400 From: Jeff Cody Message-ID: <20160810010546.GE5270@localhost.localdomain> References: <1470732609-15488-1-git-send-email-prasanna.kalever@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1470732609-15488-1-git-send-email-prasanna.kalever@redhat.com> 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: Prasanna Kumar Kalever Cc: qemu-devel@nongnu.org, armbru@redhat.com, eblake@redhat.com, areis@redhat.com, vbellur@redhat.com 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)) { > + 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? > + 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 >