[libvirt] [PATCH] Ensure virStrerror always sets an error string
strerror_r() is free to not set any error string, if the passed errno is not valid. It may, however, still return a pointer to the original passed in buffer. This resulting in random garbage from the stack being present as the error string. To reliably detect case where no error string is set, pre-init the buffer to all-zeros, and then check for empty string after calling sterror_r * src/util/virterror.c: Ensure virStrerror always sets an error string --- src/util/virterror.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-) diff --git a/src/util/virterror.c b/src/util/virterror.c index 2d7309a..eff8468 100644 --- a/src/util/virterror.c +++ b/src/util/virterror.c @@ -1267,9 +1267,13 @@ const char *virStrerror(int theerrno, char *errBuf, size_t errBufLen) int save_errno = errno; const char *ret; + memset(errBuf, 0, errBufLen); strerror_r(theerrno, errBuf, errBufLen); ret = errBuf; errno = save_errno; + + if (ret[0] == '\0') + strncpy(errBuf, _("Unknown errno"), errBufLen); return ret; } -- 1.7.4.4
2011/5/18 Daniel P. Berrange <berrange@redhat.com>:
strerror_r() is free to not set any error string, if the passed errno is not valid. It may, however, still return a pointer to the original passed in buffer. This resulting in random garbage from the stack being present as the error string.
To reliably detect case where no error string is set, pre-init the buffer to all-zeros, and then check for empty string after calling sterror_r
* src/util/virterror.c: Ensure virStrerror always sets an error string --- src/util/virterror.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/src/util/virterror.c b/src/util/virterror.c index 2d7309a..eff8468 100644 --- a/src/util/virterror.c +++ b/src/util/virterror.c @@ -1267,9 +1267,13 @@ const char *virStrerror(int theerrno, char *errBuf, size_t errBufLen) int save_errno = errno; const char *ret;
+ memset(errBuf, 0, errBufLen); strerror_r(theerrno, errBuf, errBufLen); ret = errBuf; errno = save_errno; + + if (ret[0] == '\0') + strncpy(errBuf, _("Unknown errno"), errBufLen); return ret; }
-- 1.7.4.4
I wonder in what situation you managed to notice this problem :) This doesn't pass make syntax-check because of strncpy. virStrncpy should be used instead, but this might result in not setting any error string at all when the buffer is to small. On the other hand we commonly use 1k buffers with virStrerror. Matthias
[adding bug-gnulib] On 05/18/2011 11:07 AM, Daniel P. Berrange wrote:
strerror_r() is free to not set any error string, if the passed errno is not valid. It may, however, still return a pointer to the original passed in buffer. This resulting in random garbage from the stack being present as the error string.
Indeed. However, I'm inclined to NACK the libvirt patch, because: Right now, gnulib guarantees that strerror() always gives a useful result (non-empty string for all errno values, even though POSIX allows an empty string), but the strerror_r-posix module is not making those same guarantees. Therefore, I argue that this is a bug in gnulib. We should be changing the strerror_r-posix module to guarantee sane behavior, rather than just bare-minimum compliance, even if that means replacing strerror_r on a few more platforms.
+++ b/src/util/virterror.c @@ -1267,9 +1267,13 @@ const char *virStrerror(int theerrno, char *errBuf, size_t errBufLen) int save_errno = errno; const char *ret;
+ memset(errBuf, 0, errBufLen);
That's a bit time-consuming, especially if errBufLen is MUCH bigger than the message to be printed. It would suffice to simply do *errbuf=0.
strerror_r(theerrno, errBuf, errBufLen); ret = errBuf; errno = save_errno; + + if (ret[0] == '\0') + strncpy(errBuf, _("Unknown errno"), errBufLen); return ret; }
-- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Wed, May 18, 2011 at 11:27:28AM -0600, Eric Blake wrote:
[adding bug-gnulib]
On 05/18/2011 11:07 AM, Daniel P. Berrange wrote:
strerror_r() is free to not set any error string, if the passed errno is not valid. It may, however, still return a pointer to the original passed in buffer. This resulting in random garbage from the stack being present as the error string.
Indeed. However, I'm inclined to NACK the libvirt patch, because:
Right now, gnulib guarantees that strerror() always gives a useful result (non-empty string for all errno values, even though POSIX allows an empty string), but the strerror_r-posix module is not making those same guarantees.
Therefore, I argue that this is a bug in gnulib. We should be changing the strerror_r-posix module to guarantee sane behavior, rather than just bare-minimum compliance, even if that means replacing strerror_r on a few more platforms.
Yeah, if gnulib wants to fix this, that's fine with me. 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 :|
In particular, this guarantees that virStrerror always gives a useful result. * .gnulib: Update to latest, for (lots of) strerror_r fixes. --- This patch removes the need for Daniel's proposal: https://www.redhat.com/archives/libvir-list/2011-May/msg01236.html Now gnulib guarantees that strerror_r will never leave garbage in the buffer. .gnulib | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) * .gnulib 2c25c9e...f4c4af0 (67):
docs: document recently fixed glibc printf bug closein-tests: convert to init.sh yesno-tests: convert to init.sh atexit-tests: ensure reliable exit status strerror_r-posix: Respect rules for use of AC_LIBOBJ. strerror_r: fix missing header strerror_r: fix AIX test failures strerror_r: fix Solaris test failures strerror_r: enforce POSIX recommendations test-perror2.c: avoid warning about unused variable perror: avoid spurious test failure on HP-UX tests: fix logic bug in init.sh utimensat: do not reference an out-of-scope buffer opendir-safer.c: don't clobber errno; don't close negative FD update from texinfo Fix recent ChangeLog entry. idcache: Fix module description. gnulib-tool: fix portability problem with MacOS sed hash: Simplify autoconf macro. getugroups: Fix module description. linkat: Simplify autoconf macro. linkat, renameat: Update dependencies. maint.mk: more tight_scope improvements maint.mk: generalize/improve the tight-scope rule verify: fix bug when gnulib <assert.h> is also included * doc/intprops.texi: fix typo in copyright date xgetcwd: Simplify autoconf macro. New module 'mktime-internal'. timegm: Correct mktime replacement statements. timegm: Simplify autoconf macro. clock-time: change to LGPLv2+. strerror_r: Fix comments. relocatable-prog-wrapper: Fix possible link error. relocatable-prog-wrapper: Assume strerror() exists. select: Simplify replacement idiom. mkdir-p: Simplify autoconf macro. Fix some mistakes in ChangeLog entries. strerror_r: avoid clobbering strerror on cygwin mkdtemp: Use gnulib naming conventions. strerror_r: avoid corrupting errno on Solaris strerror_r: avoid compiler warning strerror_r: simplify AIX code. test-perror: avoid spurious failure on FreeBSD strerror_r-posix: Remove unused dependencies. intprops: remove assumption about A|B representation perror: work around FreeBSD bug test-perror: check for strerror interactions test-perror: rewrite to use init script maint: replace misused "a" with "an" maint: correct misuse of "a" and "an" intprops-tests: work around HP-UX 11.23 cc bug with constants intprops: work around IRIX 6.5 cc bug with 0u - 0u + -1 intprops-tests: revert unsigned part of previous change strerror_r: Work around strerror_r() change in Cygwin 1.7.8. strerror_r: guarantee unchanged errno strerror_r: Reorder #if blocks. perror: Avoid clobbering the strerror buffer when possible. strerror_r: fix on newer cygwin strerror_r: Avoid clobbering the strerror buffer when possible. strerror_r: Fix test failure on mingw. strerror: relax test for Solaris strerror: enforce POSIX ruling on strerror(0) intprop-tests: port to older and more-pedantic compilers intprops: work around C compiler bugs intprops: TYPE_IS_INTEGER, TYPE_SIGNED not integer constant exprs strerror_r: Avoid clobbering the strerror buffer when possible. fnmatch: avoid compiler warning
diff --git a/.gnulib b/.gnulib index 2c25c9e..f4c4af0 160000 --- a/.gnulib +++ b/.gnulib @@ -1 +1 @@ -Subproject commit 2c25c9ebe8db1415bfde25f0a451767332c8cf59 +Subproject commit f4c4af09bcf3f0497dc3347ecc0a0b3c9ee1ff63 -- 1.7.4.4
2011/5/25 Eric Blake <eblake@redhat.com>:
In particular, this guarantees that virStrerror always gives a useful result.
* .gnulib: Update to latest, for (lots of) strerror_r fixes. ---
This patch removes the need for Daniel's proposal: https://www.redhat.com/archives/libvir-list/2011-May/msg01236.html
Now gnulib guarantees that strerror_r will never leave garbage in the buffer.
.gnulib | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
* .gnulib 2c25c9e...f4c4af0 (67): > docs: document recently fixed glibc printf bug [...] > fnmatch: avoid compiler warning
This also pulls in the fix for the fnmatch compile warning. ACK. Matthias
On 05/24/2011 11:23 PM, Matthias Bolte wrote:
2011/5/25 Eric Blake <eblake@redhat.com>:
In particular, this guarantees that virStrerror always gives a useful result.
* .gnulib: Update to latest, for (lots of) strerror_r fixes. ---
This patch removes the need for Daniel's proposal: https://www.redhat.com/archives/libvir-list/2011-May/msg01236.html
Now gnulib guarantees that strerror_r will never leave garbage in the buffer.
I'm impressed - I'm now up to 3 out of 3 glibc bugs reported and fixed in the last month (more often than not, I've had the misfortune of my glibc bugs report attempts being rejected with rather scathing comments that the bug is not in glibc but in my understanding, or left without a response for months on end). Leaving garbage in buf upon strerror_r failure was one of those fixes.
This also pulls in the fix for the fnmatch compile warning.
ACK.
Thanks; pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Matthias Bolte