| Message ID | 1470244580-4741-1-git-send-email-prasanna.kalever@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Cc: Eric, because there's a relation to QAPI even though the patch doesn't touch the schema. Prasanna Kumar Kalever <prasanna.kalever@redhat.com> writes: > 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() Should we use QEMU_OPT_STRING here, for the same reasons we use 'str' in the QAPI schema? > However It is a good practice to use function like parse_uint_full() > over atoi() to keep port self defended > > Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com> > --- > block/gluster.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/block/gluster.c b/block/gluster.c > index 01b479f..e2aa0f3 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; > + long long unsigned port; unsigned long long, please. > > glfs = glfs_new(gconf->volume); > if (!glfs) { > @@ -330,10 +332,16 @@ 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, 0) < 0) { Should we require decimal by passing 10 rather than 0? Port numbers are commonly decimal. For what it's worth, JSON numbers are always decimal. > + error_setg(errp, "can't convert port to a number: %s", > + server->value->u.tcp.port); Suggest "'%s' is not a valid port number". > + errno = EINVAL; > + goto out; > + } Missing: a range check. No need to get fancy, something like this might do: if (parse_uint_full(server->value->u.tcp.port, &port, 0) < 0 || port > 65535) { > 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) {
diff --git a/block/gluster.c b/block/gluster.c index 01b479f..e2aa0f3 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; + long long unsigned port; glfs = glfs_new(gconf->volume); if (!glfs) { @@ -330,10 +332,16 @@ 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, 0) < 0) { + error_setg(errp, "can't convert port to a number: %s", + 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 Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com> --- block/gluster.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)