[libvirt] [PATCH] fix errors in virReportSystemErrorFull
While looking at removing most of the remaining uses of strerror, I spotted problems in virReportSystemErrorFull. 1) it would leak "combined" 2) that allocated, written-into buffer wasn't even used So, since we'd rather avoid having to allocate at all in the error-reporting path (esp for OOM errors), I've made it automatic.
From 8550ec841b4e19049abc4b5e4a2f8a81d1824055 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Mon, 26 Jan 2009 14:59:35 +0100 Subject: [PATCH] fix errors in virReportSystemErrorFull
* src/virterror.c (virReportSystemErrorFull): Don't leak "combined". In fact, don't even attempt allocation. Do include the result of formatted print in final diagnostic. --- src/virterror.c | 13 +++++++++---- 1 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/virterror.c b/src/virterror.c index 0c66781..cfde1dc 100644 --- a/src/virterror.c +++ b/src/virterror.c @@ -1019,7 +1019,7 @@ void virReportSystemErrorFull(virConnectPtr conn, char systemError[1024]; char *theerrnostr; const char *virerr; - char *combined = NULL; + char combined[2048]; #ifdef HAVE_STRERROR_R #ifdef __USE_GNU @@ -1047,10 +1047,15 @@ void virReportSystemErrorFull(virConnectPtr conn, errorMessage[0] = '\0'; } - if (virAsprintf(&combined, "%s: %s", errorMessage, theerrnostr) < 0) - combined = theerrnostr; /* OOM, so lets just pass the strerror info as best effort */ + char *err_str; + int n = snprintf(combined, sizeof combined, "%s: %s", + errorMessage, theerrnostr); + err_str = (0 < n && n < sizeof(combined) + ? combined + /* use the strerror info as best effort */ + : theerrnostr); - virerr = virErrorMsg(VIR_ERR_SYSTEM_ERROR, (errorMessage[0] ? errorMessage : NULL)); + virerr = virErrorMsg(VIR_ERR_SYSTEM_ERROR, (err_str[0] ? err_str : NULL)); virRaiseError(conn, NULL, NULL, domcode, VIR_ERR_SYSTEM_ERROR, VIR_ERR_ERROR, virerr, errorMessage, NULL, -1, -1, virerr, errorMessage); } -- 1.6.1.1.347.g3f81d
On Mon, Jan 26, 2009 at 03:02:48PM +0100, Jim Meyering wrote:
So, since we'd rather avoid having to allocate at all in the error-reporting path (esp for OOM errors), I've made it automatic.
That doesn't really matter/help the OOM reporting scenario, because the convention for VIR_ERR_NO_MEMORY is to have a NULL fmt arg, and the virRaiseError() method will itself fail the allocation later if we were in a scenario where it mattered.
#ifdef HAVE_STRERROR_R #ifdef __USE_GNU @@ -1047,10 +1047,15 @@ void virReportSystemErrorFull(virConnectPtr conn, errorMessage[0] = '\0'; }
- if (virAsprintf(&combined, "%s: %s", errorMessage, theerrnostr) < 0) - combined = theerrnostr; /* OOM, so lets just pass the strerror info as best effort */ + char *err_str; + int n = snprintf(combined, sizeof combined, "%s: %s", + errorMessage, theerrnostr); + err_str = (0 < n && n < sizeof(combined) + ? combined + /* use the strerror info as best effort */ + : theerrnostr);
- virerr = virErrorMsg(VIR_ERR_SYSTEM_ERROR, (errorMessage[0] ? errorMessage : NULL)); + virerr = virErrorMsg(VIR_ERR_SYSTEM_ERROR, (err_str[0] ? err_str : NULL));
It is actually the next line that really need to the change to use err_str - the virErrorMsg doesn't actually care about the data in that string, only checks whether it is NULL or not, so the virErrorMsg call was technically still OK, but this next one was no good:
virRaiseError(conn, NULL, NULL, domcode, VIR_ERR_SYSTEM_ERROR, VIR_ERR_ERROR, virerr, errorMessage, NULL, -1, -1, virerr, errorMessage);
Looking at the whole method again, I think it needs to be re-written to something closer to this: const char *virSystemErrorFormat(int theerrno, char *errBuf, size_t errBufLen) { #ifdef HAVE_STRERROR_R #ifdef __USE_GNU /* Annoying linux specific API contract */ return strerror_r(theerrno, errBuf, errBufLen); #else strerror_r(theerrno, errBuf, errBufLen); return errBuf; #endif #else /* Mingw lacks strerror_r() and its strerror() is definitely not * threadsafe, so safest option is to just print the raw errno * value - we can at least reliably & safely look it up in the * header files for debug purposes */ snprintf(errBuf, errBufLen, "errno=%d", theerrno); return errBuf; #endif } void virReportSystemErrorFull(virConnectPtr conn, int domcode, int theerrno, const char *filename ATTRIBUTE_UNUSED, const char *funcname ATTRIBUTE_UNUSED, size_t linenr ATTRIBUTE_UNUSED, const char *fmt, ...) { char systemError[1024]; char msgDetailBuf[1024]; const char *errnoDetail = virSystemErrorFormat(theerrno, systemError, sizeof(systemError); const char *msg = virErrorMsg(VIR_ERR_SYSTEM_ERROR, fmt); const char *msgDetail; if (fmt) { va_list args; va_start(args, fmt); vsnprintf(msgDetailBuf, sizeof(msgDetailBuf)-1, fmt, args); va_end(args); strncat(msgDetailBuf, errnoDetail, (sizeof(msgDetailBuf)-1)-strlen(msgDetail)); msgDetail = msgDetailBuf; } else { msgDetail = errnoDetail; } virRaiseError(conn, NULL, NULL, domcode, VIR_ERR_SYSTEM_ERROR, VIR_ERR_ERROR, msg, msgDetail, NULL, -1, -1, msg, msgDetail); } Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
"Daniel P. Berrange" <berrange@redhat.com> wrote: ...
Looking at the whole method again, I think it needs to be re-written to something closer to this:
Ok, I've adapted that. Changes: - handle snprintf and vsnprintf failure - insert ": " into the result string - use stpcpy, not strcat (as a general waste-avoidance measure, -- i.e., don't traverse potentially long strings unnecessarily -- no linux- or gnulib-using project should use strcat on arbitrary strings.) That latter one required pulling in the stpcpy module from gnulib. Linux has had stpcpy for ages, and POSIX now specifies it, but Solaris 10 and others still lack it. So there are two more patches that are prerequisites for this one: - update from gnulib - add stpcpy to bootstrap's module list and pull in the required gnulib files (2 new, 3 changed) I'm checking all of this in shortly.
From 5c7812209b93654a0b236ef9529fdebe49af3972 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Mon, 26 Jan 2009 14:59:35 +0100 Subject: [PATCH] fix errors in virReportSystemErrorFull
* src/virterror.c (virStrerror): New function. (virReportSystemErrorFull): Don't leak "combined". In fact, don't even attempt allocation. Do include the result of formatted print in final diagnostic. --- src/virterror.c | 72 +++++++++++++++++++++++++++++++----------------------- 1 files changed, 41 insertions(+), 31 deletions(-) diff --git a/src/virterror.c b/src/virterror.c index 0c66781..a577002 100644 --- a/src/virterror.c +++ b/src/virterror.c @@ -1005,57 +1005,67 @@ void virReportErrorHelper(virConnectPtr conn, int domcode, int errcode, } - -void virReportSystemErrorFull(virConnectPtr conn, - int domcode, - int theerrno, - const char *filename ATTRIBUTE_UNUSED, - const char *funcname ATTRIBUTE_UNUSED, - size_t linenr ATTRIBUTE_UNUSED, - const char *fmt, ...) +static const char *virStrerror(int theerrno, char *errBuf, size_t errBufLen) { - va_list args; - char errorMessage[1024]; - char systemError[1024]; - char *theerrnostr; - const char *virerr; - char *combined = NULL; - #ifdef HAVE_STRERROR_R -#ifdef __USE_GNU +# ifdef __USE_GNU /* Annoying linux specific API contract */ - theerrnostr = strerror_r(theerrno, systemError, sizeof(systemError)); -#else - strerror_r(theerrno, systemError, sizeof(systemError)); - theerrnostr = systemError; -#endif + return strerror_r(theerrno, errBuf, errBufLen); +# else + strerror_r(theerrno, errBuf, errBufLen); + return errBuf; +# endif #else /* Mingw lacks strerror_r() and its strerror() is definitely not * threadsafe, so safest option is to just print the raw errno * value - we can at least reliably & safely look it up in the * header files for debug purposes */ - snprintf(systemError, sizeof(systemError), "errno=%d", theerrno); - theerrnostr = systemError; + int n = snprintf(errBuf, errBufLen, "errno=%d", theerrno); + return (0 < n && n < errBufLen + ? errBuf : _("internal error: buffer too small")); #endif +} + +void virReportSystemErrorFull(virConnectPtr conn, + int domcode, + int theerrno, + const char *filename ATTRIBUTE_UNUSED, + const char *funcname ATTRIBUTE_UNUSED, + size_t linenr ATTRIBUTE_UNUSED, + const char *fmt, ...) +{ + char strerror_buf[1024]; + char msgDetailBuf[1024]; + + const char *errnoDetail = virStrerror(theerrno, strerror_buf, + sizeof(strerror_buf)); + const char *msg = virErrorMsg(VIR_ERR_SYSTEM_ERROR, fmt); + const char *msgDetail = NULL; if (fmt) { + va_list args; + int n; + va_start(args, fmt); - vsnprintf(errorMessage, sizeof(errorMessage)-1, fmt, args); + n = vsnprintf(msgDetailBuf, sizeof(msgDetailBuf), fmt, args); va_end(args); - } else { - errorMessage[0] = '\0'; + + size_t len = strlen (msgDetailBuf); + if (0 <= n && n + 2 + len < sizeof (msgDetailBuf)) { + char *p = msgDetailBuf + n; + stpcpy (stpcpy (p, ": "), errnoDetail); + msgDetail = msgDetailBuf; + } } - if (virAsprintf(&combined, "%s: %s", errorMessage, theerrnostr) < 0) - combined = theerrnostr; /* OOM, so lets just pass the strerror info as best effort */ + if (!msgDetailBuf) + msgDetail = errnoDetail; - virerr = virErrorMsg(VIR_ERR_SYSTEM_ERROR, (errorMessage[0] ? errorMessage : NULL)); virRaiseError(conn, NULL, NULL, domcode, VIR_ERR_SYSTEM_ERROR, VIR_ERR_ERROR, - virerr, errorMessage, NULL, -1, -1, virerr, errorMessage); + msg, msgDetail, NULL, -1, -1, msg, msgDetail); } - void virReportOOMErrorFull(virConnectPtr conn, int domcode, const char *filename ATTRIBUTE_UNUSED, -- 1.6.1.1.363.g2a3bd
On Tue, Jan 27, 2009 at 11:28:32AM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote: ...
Looking at the whole method again, I think it needs to be re-written to something closer to this:
Ok, I've adapted that. +void virReportSystemErrorFull(virConnectPtr conn, + int domcode, + int theerrno, + const char *filename ATTRIBUTE_UNUSED, + const char *funcname ATTRIBUTE_UNUSED, + size_t linenr ATTRIBUTE_UNUSED, + const char *fmt, ...) +{ + char strerror_buf[1024]; + char msgDetailBuf[1024]; + + const char *errnoDetail = virStrerror(theerrno, strerror_buf, + sizeof(strerror_buf)); + const char *msg = virErrorMsg(VIR_ERR_SYSTEM_ERROR, fmt); + const char *msgDetail = NULL;
if (fmt) { + va_list args; + int n; + va_start(args, fmt); - vsnprintf(errorMessage, sizeof(errorMessage)-1, fmt, args); + n = vsnprintf(msgDetailBuf, sizeof(msgDetailBuf), fmt, args); va_end(args); - } else { - errorMessage[0] = '\0'; + + size_t len = strlen (msgDetailBuf); + if (0 <= n && n + 2 + len < sizeof (msgDetailBuf)) { + char *p = msgDetailBuf + n; + stpcpy (stpcpy (p, ": "), errnoDetail); + msgDetail = msgDetailBuf; + } }
- if (virAsprintf(&combined, "%s: %s", errorMessage, theerrnostr) < 0) - combined = theerrnostr; /* OOM, so lets just pass the strerror info as best effort */ + if (!msgDetailBuf) + msgDetail = errnoDetail;
This should be if (!msgDetail) - indeed just noticed the compiler warns on this cc1: warnings being treated as errors virterror.c: In function 'virReportSystemErrorFull': virterror.c:1062: error: the address of 'msgDetailBuf' will always evaluate as 'true' Dainel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
"Daniel P. Berrange" <berrange@redhat.com> wrote:
On Tue, Jan 27, 2009 at 11:28:32AM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote: ...
Looking at the whole method again, I think it needs to be re-written to something closer to this:
Ok, I've adapted that. +void virReportSystemErrorFull(virConnectPtr conn, + int domcode, + int theerrno, + const char *filename ATTRIBUTE_UNUSED, + const char *funcname ATTRIBUTE_UNUSED, + size_t linenr ATTRIBUTE_UNUSED, + const char *fmt, ...) +{ + char strerror_buf[1024]; + char msgDetailBuf[1024]; + + const char *errnoDetail = virStrerror(theerrno, strerror_buf, + sizeof(strerror_buf)); + const char *msg = virErrorMsg(VIR_ERR_SYSTEM_ERROR, fmt); + const char *msgDetail = NULL;
if (fmt) { + va_list args; + int n; + va_start(args, fmt); - vsnprintf(errorMessage, sizeof(errorMessage)-1, fmt, args); + n = vsnprintf(msgDetailBuf, sizeof(msgDetailBuf), fmt, args); va_end(args); - } else { - errorMessage[0] = '\0'; + + size_t len = strlen (msgDetailBuf); + if (0 <= n && n + 2 + len < sizeof (msgDetailBuf)) { + char *p = msgDetailBuf + n; + stpcpy (stpcpy (p, ": "), errnoDetail); + msgDetail = msgDetailBuf; + } }
- if (virAsprintf(&combined, "%s: %s", errorMessage, theerrnostr) < 0) - combined = theerrnostr; /* OOM, so lets just pass the strerror info as best effort */ + if (!msgDetailBuf) + msgDetail = errnoDetail;
This should be if (!msgDetail) - indeed just noticed the compiler warns on this
cc1: warnings being treated as errors virterror.c: In function 'virReportSystemErrorFull': virterror.c:1062: error: the address of 'msgDetailBuf' will always evaluate as 'true'
Good catch. I've just committed this:
From 2bb6830c061a580328abf4647a26b9ecc7f7eaa1 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Tue, 27 Jan 2009 13:25:16 +0100 Subject: [PATCH] virterror.c: don't read beyond end of buffer upon OOM
* src/virterror.c (virReportSystemErrorFull): Fix typo in my previous change. Patch by Daniel P. Berrange. --- src/virterror.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/virterror.c b/src/virterror.c index a577002..160fea3 100644 --- a/src/virterror.c +++ b/src/virterror.c @@ -1059,7 +1059,7 @@ void virReportSystemErrorFull(virConnectPtr conn, } } - if (!msgDetailBuf) + if (!msgDetail) msgDetail = errnoDetail; virRaiseError(conn, NULL, NULL, domcode, VIR_ERR_SYSTEM_ERROR, VIR_ERR_ERROR, -- 1.6.1.401.gf39d5
participants (2)
-
Daniel P. Berrange -
Jim Meyering