[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 1/1] block/gluster: improve defense over stri
From: |
Jeff Cody |
Subject: |
Re: [Qemu-devel] [PATCH v2 1/1] block/gluster: improve defense over string to int conversion |
Date: |
Tue, 9 Aug 2016 21:05:46 -0400 |
User-agent: |
Mutt/1.5.24 (2015-08-30) |
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 <address@hidden>
> ---
> 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
>