* [Qemu-devel] [PATCH v1 1/1] block/gluster: improve defense over string to int conversion
@ 2016-08-03 17:16 Prasanna Kumar Kalever
2016-08-04 11:44 ` Markus Armbruster
0 siblings, 1 reply; 2+ messages in thread
From: Prasanna Kumar Kalever @ 2016-08-03 17:16 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, eblake, jcody, areis, vbellur, Prasanna Kumar Kalever
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(-)
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) {
--
2.7.4
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [Qemu-devel] [PATCH v1 1/1] block/gluster: improve defense over string to int conversion
2016-08-03 17:16 [Qemu-devel] [PATCH v1 1/1] block/gluster: improve defense over string to int conversion Prasanna Kumar Kalever
@ 2016-08-04 11:44 ` Markus Armbruster
0 siblings, 0 replies; 2+ messages in thread
From: Markus Armbruster @ 2016-08-04 11:44 UTC (permalink / raw)
To: Prasanna Kumar Kalever; +Cc: qemu-devel, vbellur, jcody, areis, Eric Blake
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) {
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2016-08-04 11:44 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-03 17:16 [Qemu-devel] [PATCH v1 1/1] block/gluster: improve defense over string to int conversion Prasanna Kumar Kalever
2016-08-04 11:44 ` Markus Armbruster
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.