add_link_info() sets an error message when it fails to read /sys/class/net/$dev/operstate, but unfortunately frees the path string just prior to setting the error message, so the error string has "(null)" instead (thanks to glibc's asprintf noticing the null pointer).
Additionally, the macro used to create the log message, ERR_THROW_STRERROR() already turns errno into a string in the locally defined errbuf, but we aren't including it in the format string, so it doesn't get printed.
This patch makes sure the path isn't freed until it is really no longer needed, and adds errbuf to the log message.
This will hopefully help to find the root cause of
https://bugzilla.redhat.com/show_bug.cgi?id=1185850 --- src/dutil_linux.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/src/dutil_linux.c b/src/dutil_linux.c index 71160ee..87d0d45 100644 --- a/src/dutil_linux.c +++ b/src/dutil_linux.c @@ -1,7 +1,7 @@ /* * dutil_linux.c: Linux utility functions for driver backends. * - * Copyright (C) 2009-2012, 2014 Red Hat Inc. + * Copyright (C) 2009-2012, 2014-2015 Red Hat Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -1081,14 +1081,15 @@ static void add_link_info(struct netcf *ncf, xasprintf(&path, "/sys/class/net/%s/operstate", ifname); ERR_NOMEM(!path, ncf); state = read_file(path, &length); - FREE(path); - ERR_THROW_STRERROR(!state, ncf, EFILE, "Failed to read %s", path); + ERR_THROW_STRERROR(!state, ncf, EFILE, "Failed to read %s : %s", + path, strerror); if ((nl = strchr(state, '\n'))) *nl = 0; prop = xmlSetProp(link_node, BAD_CAST "state", BAD_CAST state); ERR_NOMEM(!prop, ncf);
if (!strcmp(state, "up")) { + FREE(path); xasprintf(&path, "/sys/class/net/%s/speed", ifname); ERR_NOMEM(path == NULL, ncf); speed = read_file(path, &length); @@ -1100,7 +1101,8 @@ static void add_link_info(struct netcf *ncf, speed = strdup("0"); ERR_NOMEM(!speed, ncf); } - ERR_THROW_STRERROR(!speed, ncf, EFILE, "Failed to read %s", path); + ERR_THROW_STRERROR(!speed, ncf, EFILE, "Failed to read %s : %s", + path, strerror); if ((nl = strchr(speed, '\n'))) *nl = 0; } else {
On 01/26/2015 02:56 PM, Laine Stump wrote:
add_link_info() sets an error message when it fails to read /sys/class/net/$dev/operstate, but unfortunately frees the path string just prior to setting the error message, so the error string has "(null)" instead (thanks to glibc's asprintf noticing the null pointer).
If the file weren't Linux-only, I'd point out that passing NULL to other libc's printf is likely to segfault - even less friendly :)
Additionally, the macro used to create the log message, ERR_THROW_STRERROR() already turns errno into a string in the locally defined errbuf, but we aren't including it in the format string, so it doesn't get printed.
This patch makes sure the path isn't freed until it is really no longer needed, and adds errbuf to the log message.
This will hopefully help to find the root cause of
https://bugzilla.redhat.com/show_bug.cgi?id=1185850
src/dutil_linux.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
@@ -1081,14 +1081,15 @@ static void add_link_info(struct netcf *ncf, xasprintf(&path, "/sys/class/net/%s/operstate", ifname); ERR_NOMEM(!path, ncf); state = read_file(path, &length);
- FREE(path);
- ERR_THROW_STRERROR(!state, ncf, EFILE, "Failed to read %s", path);
- ERR_THROW_STRERROR(!state, ncf, EFILE, "Failed to read %s : %s",
path, strerror);
'strerror' is the address of a function - printing that as a string is probably quite the wrong thing to do. I think you want 'errbuf' here.
if ((nl = strchr(state, '\n'))) *nl = 0; prop = xmlSetProp(link_node, BAD_CAST "state", BAD_CAST state); ERR_NOMEM(!prop, ncf); if (!strcmp(state, "up")) {
FREE(path); xasprintf(&path, "/sys/class/net/%s/speed", ifname); ERR_NOMEM(path == NULL, ncf); speed = read_file(path, &length);@@ -1100,7 +1101,8 @@ static void add_link_info(struct netcf *ncf, speed = strdup("0"); ERR_NOMEM(!speed, ncf); }
ERR_THROW_STRERROR(!speed, ncf, EFILE, "Failed to read %s", path);
ERR_THROW_STRERROR(!speed, ncf, EFILE, "Failed to read %s : %s",path, strerror);
and again.
On 01/26/2015 06:04 PM, Eric Blake wrote:
On 01/26/2015 02:56 PM, Laine Stump wrote:
add_link_info() sets an error message when it fails to read /sys/class/net/$dev/operstate, but unfortunately frees the path string just prior to setting the error message, so the error string has "(null)" instead (thanks to glibc's asprintf noticing the null pointer).
If the file weren't Linux-only, I'd point out that passing NULL to other libc's printf is likely to segfault - even less friendly :)
Additionally, the macro used to create the log message, ERR_THROW_STRERROR() already turns errno into a string in the locally defined errbuf, but we aren't including it in the format string, so it doesn't get printed.
This patch makes sure the path isn't freed until it is really no longer needed, and adds errbuf to the log message.
This will hopefully help to find the root cause of
https://bugzilla.redhat.com/show_bug.cgi?id=1185850
src/dutil_linux.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
@@ -1081,14 +1081,15 @@ static void add_link_info(struct netcf *ncf, xasprintf(&path, "/sys/class/net/%s/operstate", ifname); ERR_NOMEM(!path, ncf); state = read_file(path, &length);
- FREE(path);
- ERR_THROW_STRERROR(!state, ncf, EFILE, "Failed to read %s", path);
- ERR_THROW_STRERROR(!state, ncf, EFILE, "Failed to read %s : %s",
path, strerror);'strerror' is the address of a function - printing that as a string is probably quite the wrong thing to do. I think you want 'errbuf' here.
Oops. Yeah, that's what my mind was thinking, but my fingers were in a hurry. Proof once again that it's never good to push without a review.
if ((nl = strchr(state, '\n'))) *nl = 0; prop = xmlSetProp(link_node, BAD_CAST "state", BAD_CAST state); ERR_NOMEM(!prop, ncf); if (!strcmp(state, "up")) {
FREE(path); xasprintf(&path, "/sys/class/net/%s/speed", ifname); ERR_NOMEM(path == NULL, ncf); speed = read_file(path, &length);@@ -1100,7 +1101,8 @@ static void add_link_info(struct netcf *ncf, speed = strdup("0"); ERR_NOMEM(!speed, ncf); }
ERR_THROW_STRERROR(!speed, ncf, EFILE, "Failed to read %s", path);
ERR_THROW_STRERROR(!speed, ncf, EFILE, "Failed to read %s : %s",path, strerror);and again.
I assume ACK once I fix the bonehead finger fumble?
On 01/26/2015 07:00 PM, Laine Stump wrote:
On 01/26/2015 06:04 PM, Eric Blake wrote:
On 01/26/2015 02:56 PM, Laine Stump wrote:
add_link_info() sets an error message when it fails to read /sys/class/net/$dev/operstate, but unfortunately frees the path string just prior to setting the error message, so the error string has "(null)" instead (thanks to glibc's asprintf noticing the null pointer).
- ERR_THROW_STRERROR(!state, ncf, EFILE, "Failed to read %s", path);
- ERR_THROW_STRERROR(!state, ncf, EFILE, "Failed to read %s : %s",
path, strerror);'strerror' is the address of a function - printing that as a string is probably quite the wrong thing to do. I think you want 'errbuf' here.
Oops. Yeah, that's what my mind was thinking, but my fingers were in a hurry. Proof once again that it's never good to push without a review.
I assume ACK once I fix the bonehead finger fumble?
Yes, the rest looks good, so consider this as ACK.
netcf-devel@lists.fedorahosted.org
-
Eric Blake -
Laine Stump