[libvirt] [PATCH] qemu: Check for negative port values in network drive configuration
We interpret port values as signed int (convert them from char *), so if a negative value is provided in network disk's configuration, we accept it as valid, however there's an 'unknown cause' error raised later. This error is only accidental because we return the port value in the return code. This patch adds just a minor tweak to the already existing check so we reject negative values the same way as we reject non-numerical strings. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1163553 --- src/qemu/qemu_command.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 743d6f0..c1e9559 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2954,7 +2954,7 @@ qemuNetworkDriveGetPort(int protocol, int ret = 0; if (port) { - if (virStrToLong_i(port, NULL, 10, &ret) < 0) { + if (virStrToLong_i(port, NULL, 10, &ret) < 0 || ret < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("failed to parse port number '%s'"), port); -- 1.9.3
On 02/19/2015 08:53 AM, Erik Skultety wrote:
We interpret port values as signed int (convert them from char *), so if a negative value is provided in network disk's configuration, we accept it as valid, however there's an 'unknown cause' error raised later. This error is only accidental because we return the port value in the return code. This patch adds just a minor tweak to the already existing check so we reject negative values the same way as we reject non-numerical strings.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1163553 --- src/qemu/qemu_command.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 743d6f0..c1e9559 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2954,7 +2954,7 @@ qemuNetworkDriveGetPort(int protocol, int ret = 0;
if (port) { - if (virStrToLong_i(port, NULL, 10, &ret) < 0) { + if (virStrToLong_i(port, NULL, 10, &ret) < 0 || ret < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("failed to parse port number '%s'"), port);
Won't this still allow wraparound (an extremely large negative input that gets parsed as positive)? Wouldn't it be better to switch to virStrToLong_uip to force a positive number parse? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Thu, Feb 19, 2015 at 04:28:44PM -0700, Eric Blake wrote:
On 02/19/2015 08:53 AM, Erik Skultety wrote:
We interpret port values as signed int (convert them from char *), so if a negative value is provided in network disk's configuration, we accept it as valid, however there's an 'unknown cause' error raised later. This error is only accidental because we return the port value in the return code. This patch adds just a minor tweak to the already existing check so we reject negative values the same way as we reject non-numerical strings.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1163553 --- src/qemu/qemu_command.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 743d6f0..c1e9559 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2954,7 +2954,7 @@ qemuNetworkDriveGetPort(int protocol, int ret = 0;
if (port) { - if (virStrToLong_i(port, NULL, 10, &ret) < 0) { + if (virStrToLong_i(port, NULL, 10, &ret) < 0 || ret < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("failed to parse port number '%s'"), port);
Won't this still allow wraparound (an extremely large negative input that gets parsed as positive)? Wouldn't it be better to switch to virStrToLong_uip to force a positive number parse?
No it won't, virStrToLong_*() functions handle this properly. We use this parsing for other integers as well. I don't know what I would do if our *integer* variant isn't good for parsing *integers* ;-) So I'd say ACK from my POV.
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Feb 19, 2015 at 16:28:44 -0700, Eric Blake wrote:
On 02/19/2015 08:53 AM, Erik Skultety wrote:
We interpret port values as signed int (convert them from char *), so if a negative value is provided in network disk's configuration, we accept it as valid, however there's an 'unknown cause' error raised later. This error is only accidental because we return the port value in the return code. This patch adds just a minor tweak to the already existing check so we reject negative values the same way as we reject non-numerical strings.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1163553 --- src/qemu/qemu_command.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 743d6f0..c1e9559 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2954,7 +2954,7 @@ qemuNetworkDriveGetPort(int protocol, int ret = 0;
if (port) { - if (virStrToLong_i(port, NULL, 10, &ret) < 0) { + if (virStrToLong_i(port, NULL, 10, &ret) < 0 || ret < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("failed to parse port number '%s'"), port);
Won't this still allow wraparound (an extremely large negative input that gets parsed as positive)? Wouldn't it be better to switch to virStrToLong_uip to force a positive number parse?
The wrap-around case should be handled fine with the existing helper for signed numbers. Actually, when using virStrToLong_uip, we'd need to make sure that the number won't get wrapped as the function is returning an int due to error reporting. I think Erik's patch is actually fine in the context of the code. Peter
participants (4)
-
Eric Blake -
Erik Skultety -
Martin Kletzander -
Peter Krempa