[libvirt] [PATCH] nwfilter: Fix sscanf off-by-one error in virNWFilterSnoopLeaseFileLoad
We allocate 16 bytes for IPv4 address and 55 bytes for interface key, therefore we should read up to 15/54 bytes and let the last byte reserved for terminating null byte in sscanf. https://bugzilla.redhat.com/show_bug.cgi?id=1226400 --- src/nwfilter/nwfilter_dhcpsnoop.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c index 6da8983..f331e22 100644 --- a/src/nwfilter/nwfilter_dhcpsnoop.c +++ b/src/nwfilter/nwfilter_dhcpsnoop.c @@ -1958,8 +1958,8 @@ virNWFilterSnoopLeaseFileLoad(void) break; } ln++; - /* key len 55 = "VMUUID"+'-'+"MAC" */ - if (sscanf(line, "%u %55s %16s %16s", &ipl.timeout, + /* key len 54 = "VMUUID"+'-'+"MAC" */ + if (sscanf(line, "%u %54s %15s %15s", &ipl.timeout, ifkey, ipstr, srvstr) < 4) { virReportError(VIR_ERR_INTERNAL_ERROR, _("virNWFilterSnoopLeaseFileLoad lease file " -- 1.9.3
On Tue, Jun 02, 2015 at 10:18:34AM +0200, Erik Skultety wrote:
We allocate 16 bytes for IPv4 address and 55 bytes for interface key, therefore we should read up to 15/54 bytes and let the last byte reserved for terminating null byte in sscanf.
https://bugzilla.redhat.com/show_bug.cgi?id=1226400 --- src/nwfilter/nwfilter_dhcpsnoop.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c index 6da8983..f331e22 100644 --- a/src/nwfilter/nwfilter_dhcpsnoop.c +++ b/src/nwfilter/nwfilter_dhcpsnoop.c @@ -1958,8 +1958,8 @@ virNWFilterSnoopLeaseFileLoad(void) break; } ln++; - /* key len 55 = "VMUUID"+'-'+"MAC" */ - if (sscanf(line, "%u %55s %16s %16s", &ipl.timeout, + /* key len 54 = "VMUUID"+'-'+"MAC" */ + if (sscanf(line, "%u %54s %15s %15s", &ipl.timeout, ifkey, ipstr, srvstr) < 4) {
We initialize ifkey as char ifkey[VIR_IFKEY_LEN], so it might be nicer to call: if (sscanf(line, "%u %*s %*s %*s", &ipl.timeout, VIR_IFKEY_LEN - 1, ifkey, INET_ADDRSTRLEN - 1, ipstr, INET_ADDRSTRLEN - 1, srvstr) < 4) { ... But what you have is enough, so ACK to that.
On 06/02/2015 11:35 AM, Martin Kletzander wrote:
On Tue, Jun 02, 2015 at 10:18:34AM +0200, Erik Skultety wrote:
We allocate 16 bytes for IPv4 address and 55 bytes for interface key, therefore we should read up to 15/54 bytes and let the last byte reserved for terminating null byte in sscanf.
https://bugzilla.redhat.com/show_bug.cgi?id=1226400 --- src/nwfilter/nwfilter_dhcpsnoop.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c index 6da8983..f331e22 100644 --- a/src/nwfilter/nwfilter_dhcpsnoop.c +++ b/src/nwfilter/nwfilter_dhcpsnoop.c @@ -1958,8 +1958,8 @@ virNWFilterSnoopLeaseFileLoad(void) break; } ln++; - /* key len 55 = "VMUUID"+'-'+"MAC" */ - if (sscanf(line, "%u %55s %16s %16s", &ipl.timeout, + /* key len 54 = "VMUUID"+'-'+"MAC" */ + if (sscanf(line, "%u %54s %15s %15s", &ipl.timeout, ifkey, ipstr, srvstr) < 4) {
We initialize ifkey as char ifkey[VIR_IFKEY_LEN], so it might be nicer to call:
if (sscanf(line, "%u %*s %*s %*s", &ipl.timeout, VIR_IFKEY_LEN - 1, ifkey, INET_ADDRSTRLEN - 1, ipstr, INET_ADDRSTRLEN - 1, srvstr) < 4) { ...
I'd like to disagree --> 'error: too many arguments for format', perhaps you've mistaken this for printf where man says: "Instead of a decimal digit string one may write "*" or "*m$" (for some decimal integer m) to specify that the field width is given in the next argument, or in the m-th argument, respectively, which must be of type int." This however is different for scanf, where '*' is defined as assignment-suppression character
But what you have is enough, so ACK to that. Thanks for the ACK though.
Regards, Erik
On Tue, Jun 02, 2015 at 12:21:32PM +0200, Erik Skultety wrote:
On 06/02/2015 11:35 AM, Martin Kletzander wrote:
On Tue, Jun 02, 2015 at 10:18:34AM +0200, Erik Skultety wrote:
We allocate 16 bytes for IPv4 address and 55 bytes for interface key, therefore we should read up to 15/54 bytes and let the last byte reserved for terminating null byte in sscanf.
https://bugzilla.redhat.com/show_bug.cgi?id=1226400 --- src/nwfilter/nwfilter_dhcpsnoop.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c index 6da8983..f331e22 100644 --- a/src/nwfilter/nwfilter_dhcpsnoop.c +++ b/src/nwfilter/nwfilter_dhcpsnoop.c @@ -1958,8 +1958,8 @@ virNWFilterSnoopLeaseFileLoad(void) break; } ln++; - /* key len 55 = "VMUUID"+'-'+"MAC" */ - if (sscanf(line, "%u %55s %16s %16s", &ipl.timeout, + /* key len 54 = "VMUUID"+'-'+"MAC" */ + if (sscanf(line, "%u %54s %15s %15s", &ipl.timeout, ifkey, ipstr, srvstr) < 4) {
We initialize ifkey as char ifkey[VIR_IFKEY_LEN], so it might be nicer to call:
if (sscanf(line, "%u %*s %*s %*s", &ipl.timeout, VIR_IFKEY_LEN - 1, ifkey, INET_ADDRSTRLEN - 1, ipstr, INET_ADDRSTRLEN - 1, srvstr) < 4) { ...
I'd like to disagree --> 'error: too many arguments for format', perhaps you've mistaken this for printf where man says: "Instead of a decimal digit string one may write "*" or "*m$" (for some decimal integer m) to specify that the field width is given in the next argument, or in the m-th argument, respectively, which must be of type int."
Yep, for sure that's what I did, sorry ;)
This however is different for scanf, where '*' is defined as assignment-suppression character
But what you have is enough, so ACK to that. Thanks for the ACK though.
You're welcome
Regards, Erik
participants (2)
-
Erik Skultety -
Martin Kletzander