[libvirt] [PATCH] nodeinfo: Make sure we always reset errno before calling readdir
We must always reset errno to 0 even if we do 'continue'. This fixes runtime with musl libc which will set errno on sscanf. Signed-off-by: Natanael Copa <ncopa@alpinelinux.org> --- src/nodeinfo.c | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 53ba716..8d3214e 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -452,8 +452,7 @@ virNodeParseNode(const char *node, /* enumerate sockets in the node */ CPU_ZERO(&sock_map); - errno = 0; - while ((cpudirent = readdir(cpudir))) { + for (errno = 0; (cpudirent = readdir(cpudir)); errno = 0) { if (sscanf(cpudirent->d_name, "cpu%u", &cpu) != 1) continue; @@ -470,8 +469,6 @@ virNodeParseNode(const char *node, if (sock > sock_max) sock_max = sock; - - errno = 0; } if (errno) { @@ -490,8 +487,7 @@ virNodeParseNode(const char *node, /* iterate over all CPU's in the node */ rewinddir(cpudir); - errno = 0; - while ((cpudirent = readdir(cpudir))) { + for (errno = 0; (cpudirent = readdir(cpudir)); errno = 0) { if (sscanf(cpudirent->d_name, "cpu%u", &cpu) != 1) continue; @@ -530,8 +526,6 @@ virNodeParseNode(const char *node, if (siblings > *threads) *threads = siblings; - - errno = 0; } if (errno) { @@ -672,8 +666,7 @@ int linuxNodeInfoCPUPopulate(FILE *cpuinfo, goto fallback; } - errno = 0; - while ((nodedirent = readdir(nodedir))) { + for (errno = 0; (nodedirent = readdir(nodedir)); errno = 0) { if (sscanf(nodedirent->d_name, "node%u", &node) != 1) continue; @@ -699,8 +692,6 @@ int linuxNodeInfoCPUPopulate(FILE *cpuinfo, if (threads > nodeinfo->threads) nodeinfo->threads = threads; - - errno = 0; } if (errno) { -- 1.9.1
On 04/10/2014 08:04 AM, Natanael Copa wrote:
We must always reset errno to 0 even if we do 'continue'.
This fixes runtime with musl libc which will set errno on sscanf.
Signed-off-by: Natanael Copa <ncopa@alpinelinux.org> --- src/nodeinfo.c | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-)
diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 53ba716..8d3214e 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -452,8 +452,7 @@ virNodeParseNode(const char *node,
/* enumerate sockets in the node */ CPU_ZERO(&sock_map); - errno = 0; - while ((cpudirent = readdir(cpudir))) { + for (errno = 0; (cpudirent = readdir(cpudir)); errno = 0) { if (sscanf(cpudirent->d_name, "cpu%u", &cpu) != 1) continue;
Good catch. However, the code is still buggy even after your fix. We are missing: if (errno) break; before attempting the sscanf. Furthermore, sscanf() is undefined on overflow; while the cpudir is unlikely to be giving us integers that overflow, it would be nicer to not use sscanf in the first place. It's more than just the improper use of readdir that needs fixing here. I'm debating whether to push this now and fix the rest as a followup, or whether to do a v2 that fixes it all at once. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Thu, Apr 10, 2014 at 08:24:20AM -0600, Eric Blake wrote:
On 04/10/2014 08:04 AM, Natanael Copa wrote:
We must always reset errno to 0 even if we do 'continue'.
This fixes runtime with musl libc which will set errno on sscanf.
Signed-off-by: Natanael Copa <ncopa@alpinelinux.org> --- src/nodeinfo.c | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-)
diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 53ba716..8d3214e 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -452,8 +452,7 @@ virNodeParseNode(const char *node,
/* enumerate sockets in the node */ CPU_ZERO(&sock_map); - errno = 0; - while ((cpudirent = readdir(cpudir))) { + for (errno = 0; (cpudirent = readdir(cpudir)); errno = 0) { if (sscanf(cpudirent->d_name, "cpu%u", &cpu) != 1) continue;
Good catch. However, the code is still buggy even after your fix. We are missing:
if (errno) break;
before attempting the sscanf. Furthermore, sscanf() is undefined on overflow; while the cpudir is unlikely to be giving us integers that overflow, it would be nicer to not use sscanf in the first place. It's more than just the improper use of readdir that needs fixing here.
I'm debating whether to push this now and fix the rest as a followup, or whether to do a v2 that fixes it all at once.
More generally, just about every use of readdir() in our codebase is broken in this way. IMHO it is time to create virFileReaddir() which wraps it and doesn't overload the return value to include both error and data values ie use an output parameter for the returned dir instance, and raise full libvirt errors upon error. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Thu, 10 Apr 2014 08:24:20 -0600 Eric Blake <eblake@redhat.com> wrote:
On 04/10/2014 08:04 AM, Natanael Copa wrote:
We must always reset errno to 0 even if we do 'continue'.
This fixes runtime with musl libc which will set errno on sscanf.
Signed-off-by: Natanael Copa <ncopa@alpinelinux.org> --- src/nodeinfo.c | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-)
diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 53ba716..8d3214e 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -452,8 +452,7 @@ virNodeParseNode(const char *node,
/* enumerate sockets in the node */ CPU_ZERO(&sock_map); - errno = 0; - while ((cpudirent = readdir(cpudir))) { + for (errno = 0; (cpudirent = readdir(cpudir)); errno = 0) { if (sscanf(cpudirent->d_name, "cpu%u", &cpu) != 1) continue;
Good catch. However, the code is still buggy even after your fix. We are missing:
if (errno) break;
before attempting the sscanf.
Why? if readdir fails it should return NULL and the for() loop condition should break the loop. If readdir does not return NULL it didn't fail and errno value is undefined. I suppose we could use helper function to make it more readable: static struct dirent *virReaddir(DIR *dirp) { errno = 0; return readdir(dirp); } ... while ((cpudirent = virReaddir(cpudir))
Furthermore, sscanf() is undefined on overflow; while the cpudir is unlikely to be giving us integers that overflow, it would be nicer to not use sscanf in the first place. It's more than just the improper use of readdir that needs fixing here.
I'm debating whether to push this now and fix the rest as a followup, or whether to do a v2 that fixes it all at once.
I'd say fix the current bug first and do the clean up in separate commit. -nc
On 04/10/2014 08:38 AM, Natanael Copa wrote:
- errno = 0; - while ((cpudirent = readdir(cpudir))) { + for (errno = 0; (cpudirent = readdir(cpudir)); errno = 0) { if (sscanf(cpudirent->d_name, "cpu%u", &cpu) != 1) continue;
Good catch. However, the code is still buggy even after your fix. We are missing:
if (errno) break;
before attempting the sscanf.
Why? if readdir fails it should return NULL and the for() loop condition should break the loop. If readdir does not return NULL it didn't fail and errno value is undefined.
Uggh, you're right. Yet another reason why I hate the readdir() interface.
I suppose we could use helper function to make it more readable:
static struct dirent *virReaddir(DIR *dirp) { errno = 0; return readdir(dirp); }
Or maybe: int virReaddir(DIR *dirp, struct dirent **ent) { errno = 0; *ent = readdir(dirp); if (!*ent && errno) { virReportSystemError(errno, _("unable to read directory")) return -1; } return *ent ? 1 : 0; } and used as: while ((ret = virReaddir(dirp, &entry)) > 0) { process entry } if (ret < 0) goto error;
while ((cpudirent = virReaddir(cpudir))
Furthermore, sscanf() is undefined on overflow; while the cpudir is unlikely to be giving us integers that overflow, it would be nicer to not use sscanf in the first place. It's more than just the improper use of readdir that needs fixing here.
I'm debating whether to push this now and fix the rest as a followup, or whether to do a v2 that fixes it all at once.
I'd say fix the current bug first and do the clean up in separate commit.
Saving sscanf() abuse cleanup for a separate commit is just fine. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Thu, 10 Apr 2014 09:22:42 -0600 Eric Blake <eblake@redhat.com> wrote:
On 04/10/2014 08:38 AM, Natanael Copa wrote:
- errno = 0; - while ((cpudirent = readdir(cpudir))) { + for (errno = 0; (cpudirent = readdir(cpudir)); errno = 0) { if (sscanf(cpudirent->d_name, "cpu%u", &cpu) != 1) continue;
...
I suppose we could use helper function to make it more readable:
static struct dirent *virReaddir(DIR *dirp) { errno = 0; return readdir(dirp); }
Or maybe:
int virReaddir(DIR *dirp, struct dirent **ent) { errno = 0; *ent = readdir(dirp); if (!*ent && errno) { virReportSystemError(errno, _("unable to read directory")) return -1; } return *ent ? 1 : 0; }
and used as:
while ((ret = virReaddir(dirp, &entry)) > 0) { process entry } if (ret < 0) goto error;
This looks better yes. Should I prepare a new patch with this? And grep for more readdirs? -nc
On 04/10/2014 02:52 PM, Natanael Copa wrote:
I suppose we could use helper function to make it more readable:
int virReaddir(DIR *dirp, struct dirent **ent) { errno = 0; *ent = readdir(dirp); if (!*ent && errno) { virReportSystemError(errno, _("unable to read directory")) return -1; } return *ent ? 1 : 0;
or shorter, return !!*ent;
}
and used as:
while ((ret = virReaddir(dirp, &entry)) > 0) { process entry } if (ret < 0) goto error;
This looks better yes.
Should I prepare a new patch with this? And grep for more readdirs?
Sure; and while at it, update cfg.mk to add a new syntax check to enforce this style. We've wrapped other awkward standard functions that require errno manipulation for correct use (such as strtol and getpwuid_r), precisely because code is more maintainable when we can enforce that the awkward functions are always used correctly. Food for thought: depending on how many clients you find, it may be worth a version of virReaddir() that takes a flag argument, for auto-filtering '.' and '..' entries. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Thu, 10 Apr 2014 15:53:57 -0600 Eric Blake <eblake@redhat.com> wrote:
On 04/10/2014 02:52 PM, Natanael Copa wrote:
I suppose we could use helper function to make it more readable:
int virReaddir(DIR *dirp, struct dirent **ent) { errno = 0; *ent = readdir(dirp); if (!*ent && errno) { virReportSystemError(errno, _("unable to read directory")) return -1; } return *ent ? 1 : 0;
or shorter, return !!*ent;
}
and used as:
while ((ret = virReaddir(dirp, &entry)) > 0) { process entry } if (ret < 0) goto error;
This looks better yes.
Should I prepare a new patch with this? And grep for more readdirs?
Sure; and while at it, update cfg.mk to add a new syntax check to enforce this style. We've wrapped other awkward standard functions that require errno manipulation for correct use (such as strtol and getpwuid_r), precisely because code is more maintainable when we can enforce that the awkward functions are always used correctly.
I found a fairly nice example of readdir used correctly in vircgroup.c. We could do it like that too. It means less intrusive changes. And we avoid changing the current error messages from "Unable to iterate over TAP/loop/NBD devices" to "unable to read directory". We could alternatively pass another option with the dirname for error messages.
Food for thought: depending on how many clients you find, it may be worth a version of virReaddir() that takes a flag argument, for auto-filtering '.' and '..' entries.
I found a virDirCreate function so if we still want do the readdir, then we should probably do: virDirOpen, virDirRead and virDirClose. -nc
On Fri, Apr 11, 2014 at 09:48:41AM +0000, Natanael Copa wrote:
On Thu, 10 Apr 2014 15:53:57 -0600 Eric Blake <eblake@redhat.com> wrote:
On 04/10/2014 02:52 PM, Natanael Copa wrote:
I suppose we could use helper function to make it more readable:
int virReaddir(DIR *dirp, struct dirent **ent) { errno = 0; *ent = readdir(dirp); if (!*ent && errno) { virReportSystemError(errno, _("unable to read directory")) return -1; } return *ent ? 1 : 0;
or shorter, return !!*ent;
}
and used as:
while ((ret = virReaddir(dirp, &entry)) > 0) { process entry } if (ret < 0) goto error;
This looks better yes.
Should I prepare a new patch with this? And grep for more readdirs?
Sure; and while at it, update cfg.mk to add a new syntax check to enforce this style. We've wrapped other awkward standard functions that require errno manipulation for correct use (such as strtol and getpwuid_r), precisely because code is more maintainable when we can enforce that the awkward functions are always used correctly.
I found a fairly nice example of readdir used correctly in vircgroup.c. We could do it like that too. It means less intrusive changes. And we avoid changing the current error messages from "Unable to iterate over TAP/loop/NBD devices" to "unable to read directory". We could alternatively pass another option with the dirname for error messages.
The root problem here is that the readdir() API is incredibly badly designed. If we continue to use readdir() directly, we're going to hit this bug over and over and over and over and over again. We should introduce a wrapper that makes it harder for people to write incorrect code, so we can solve this bug once now and be done with it. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Thu, 10 Apr 2014 15:53:57 -0600 Eric Blake <eblake@redhat.com> wrote:
On 04/10/2014 02:52 PM, Natanael Copa wrote:
I suppose we could use helper function to make it more readable:
int virReaddir(DIR *dirp, struct dirent **ent) { errno = 0; *ent = readdir(dirp); if (!*ent && errno) { virReportSystemError(errno, _("unable to read directory")) return -1; } return *ent ? 1 : 0;
or shorter, return !!*ent;
}
and used as:
while ((ret = virReaddir(dirp, &entry)) > 0) { process entry } if (ret < 0) goto error;
This looks better yes.
Should I prepare a new patch with this? And grep for more readdirs?
Sure; and while at it, update cfg.mk to add a new syntax check to enforce this style. We've wrapped other awkward standard functions that require errno manipulation for correct use (such as strtol and getpwuid_r), precisely because code is more maintainable when we can enforce that the awkward functions are always used correctly.
I started with virDirOpen/Read/Close API but it turned out to be a can of worms. Some places we apparently check if closedir() works and logs errors. Some places not. Not big deal since virDirClose wrapper can make sure errors are always logged. But some places we ignore errors for virDirOpen (conf/domain_conf.c). this means that we need the virDirOpen optionally support 'ignore missing dirs' or we simply drop to use virDirOpen in this case. I am starting to think that we should probably just fix the way opendir is used. Its not that hard to do it "right": int rc = -1; for (;;) { struct dirent *ent; errno = 0; ent = readdir(dirp); if (ent == NULL) { if ((rc = -errno)) { /* log error */ } break; } ... } Or do you prefer that I continue bang my head into virDirOpen/Read/Close API? Or should we just keep current use of opendir and closedir and only implement virDirRead? -nc
On Tue, Apr 15, 2014 at 11:27:12AM +0200, Natanael Copa wrote:
On Thu, 10 Apr 2014 15:53:57 -0600 Eric Blake <eblake@redhat.com> wrote:
On 04/10/2014 02:52 PM, Natanael Copa wrote:
I suppose we could use helper function to make it more readable:
int virReaddir(DIR *dirp, struct dirent **ent) { errno = 0; *ent = readdir(dirp); if (!*ent && errno) { virReportSystemError(errno, _("unable to read directory")) return -1; } return *ent ? 1 : 0;
or shorter, return !!*ent;
}
and used as:
while ((ret = virReaddir(dirp, &entry)) > 0) { process entry } if (ret < 0) goto error;
This looks better yes.
Should I prepare a new patch with this? And grep for more readdirs?
Sure; and while at it, update cfg.mk to add a new syntax check to enforce this style. We've wrapped other awkward standard functions that require errno manipulation for correct use (such as strtol and getpwuid_r), precisely because code is more maintainable when we can enforce that the awkward functions are always used correctly.
I started with virDirOpen/Read/Close API but it turned out to be a can of worms.
Some places we apparently check if closedir() works and logs errors. Some places not. Not big deal since virDirClose wrapper can make sure errors are always logged.
But some places we ignore errors for virDirOpen (conf/domain_conf.c). this means that we need the virDirOpen optionally support 'ignore missing dirs' or we simply drop to use virDirOpen in this case.
I am starting to think that we should probably just fix the way opendir is used. Its not that hard to do it "right":
int rc = -1; for (;;) { struct dirent *ent; errno = 0; ent = readdir(dirp); if (ent == NULL) { if ((rc = -errno)) { /* log error */ } break; }
... }
Or do you prefer that I continue bang my head into virDirOpen/Read/Close API?
Or should we just keep current use of opendir and closedir and only implement virDirRead?
I'd only both with virDirRead, since that's the troublesome function Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Natanael Copa