12:07 p.m.
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
12:18 p.m.
2011/5/18 Daniel P. Berrange <berrange(a)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
12:27 p.m.
[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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
1:23 p.m.
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 :|
6:41 p.m.
New subject: [libvirt] [PATCH] build: avoid strerror_r pitfalls
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
12:23 a.m.
New subject: [libvirt] [PATCH] build: avoid strerror_r pitfalls
2011/5/25 Eric Blake <eblake(a)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
11:13 a.m.
New subject: [libvirt] [PATCH] build: avoid strerror_r pitfalls
On 05/24/2011 11:23 PM, Matthias Bolte wrote:
2011/5/25 Eric Blake <eblake(a)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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
4931
days inactive
4938
days old
6 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Matthias Bolte