| Message ID | 1470732609-15488-1-git-send-email-prasanna.kalever@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <prasanna.kalever@redhat.com> > --- > 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 >
Jeff Cody <jcody@redhat.com> 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 <prasanna.kalever@redhat.com> >> --- >> 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 <armbru@redhat.com>
On Wed, Aug 10, 2016 at 09:13:48AM +0200, Markus Armbruster wrote: > Jeff Cody <jcody@redhat.com> 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 <prasanna.kalever@redhat.com> > >> --- > >> 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 <armbru@redhat.com> Thanks, Applied to my block branch: git://github.com/codyprime/qemu-kvm-jtc.git block [Removed spurious parenthesis] -Jeff
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; + 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) {
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 <prasanna.kalever@redhat.com> --- v1: Initial patch v2: Address comments on v1 given by Markus --- block/gluster.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)