[libvirt] [PATCH v2] udev: fix crash in libudev logging
Call virLogVMessage instead of virLogMessage, since libudev called us with a va_list object, not a list of arguments. Honor message priority and strip the trailing newline. https://bugzilla.redhat.com/show_bug.cgi?id=969152 --- v1: https://www.redhat.com/archives/libvir-list/2013-June/msg00349.html v2: don't ignore priority and strip the trailing newline. src/libvirt_private.syms | 2 ++ src/node_device/node_device_udev.c | 42 ++++++++++++++++++++++++++++++-------- src/util/virlog.c | 25 +++++++++++++++++++++++ src/util/virlog.h | 1 + 4 files changed, 61 insertions(+), 9 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 042081f..ee517c1 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1480,12 +1480,14 @@ virLogMessage; virLogParseDefaultPriority; virLogParseFilters; virLogParseOutputs; +virLogPriorityFromSyslog; virLogProbablyLogMessage; virLogReset; virLogSetBufferSize; virLogSetDefaultPriority; virLogSetFromEnv; virLogUnlock; +virLogVMessage; # util/virmacaddr.h diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 620cd58..b462549 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -348,15 +348,38 @@ static int udevGenerateDeviceName(struct udev_device *device, } -static void udevLogFunction(struct udev *udev ATTRIBUTE_UNUSED, - int priority ATTRIBUTE_UNUSED, - const char *file, - int line, - const char *fn, - const char *fmt, - va_list args) +typedef void (*udevLogFunctionPtr)(struct udev *udev, + int priority, + const char *file, + int line, + const char *fn, + const char *format, + va_list args); + +static void +ATTRIBUTE_FMT_PRINTF(6,0) +udevLogFunction(struct udev *udev ATTRIBUTE_UNUSED, + int priority, + const char *file, + int line, + const char *fn, + const char *fmt, + va_list args) { - VIR_ERROR_INT(VIR_LOG_FROM_LIBRARY, file, line, fn, fmt, args); + virBuffer buf = VIR_BUFFER_INITIALIZER; + const char *format = NULL; + + virBufferAdd(&buf, fmt, -1); + virBufferTrim(&buf, "\n", -1); + + format = virBufferContentAndReset(&buf); + + if (format) + virLogVMessage(VIR_LOG_FROM_LIBRARY, + virLogPriorityFromSyslog(priority), + file, line, fn, NULL, format, args); + + VIR_FREE(format); } @@ -1672,7 +1695,8 @@ static int nodeStateInitialize(bool privileged ATTRIBUTE_UNUSED, * its return value. */ udev = udev_new(); - udev_set_log_fn(udev, udevLogFunction); + /* cast to get rid of missing-format-attribute warning */ + udev_set_log_fn(udev, (udevLogFunctionPtr) udevLogFunction); priv->udev_monitor = udev_monitor_new_from_netlink(udev, "udev"); if (priv->udev_monitor == NULL) { diff --git a/src/util/virlog.c b/src/util/virlog.c index 739a274..064f8e8 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -1264,6 +1264,31 @@ static int virLogAddOutputToJournald(int priority) return 0; } # endif /* USE_JOURNALD */ + +int virLogPriorityFromSyslog(int priority) +{ + switch (priority) { + case LOG_EMERG: + case LOG_ALERT: + case LOG_CRIT: + case LOG_ERR: + return VIR_LOG_ERROR; + case LOG_WARNING: + case LOG_NOTICE: + return VIR_LOG_WARN; + case LOG_INFO: + return VIR_LOG_INFO; + case LOG_DEBUG: + return VIR_LOG_DEBUG; + } + return VIR_LOG_ERROR; +} + +#else /* HAVE_SYSLOG_H */ +int virLogPriorityFromSyslog(int priority ATTRIBUTE_UNUSED) +{ + return VIR_LOG_ERROR; +} #endif /* HAVE_SYSLOG_H */ #define IS_SPACE(cur) \ diff --git a/src/util/virlog.h b/src/util/virlog.h index 6b83245..7db1657 100644 --- a/src/util/virlog.h +++ b/src/util/virlog.h @@ -171,6 +171,7 @@ extern int virLogReset(void); extern int virLogParseDefaultPriority(const char *priority); extern int virLogParseFilters(const char *filters); extern int virLogParseOutputs(const char *output); +extern int virLogPriorityFromSyslog(int priority); extern void virLogMessage(virLogSource src, virLogPriority priority, const char *filename, -- 1.8.1.5
On 06/13/2013 09:56 PM, Ján Tomko wrote:
Call virLogVMessage instead of virLogMessage, since libudev called us with a va_list object, not a list of arguments.
Honor message priority and strip the trailing newline.
https://bugzilla.redhat.com/show_bug.cgi?id=969152 --- v1: https://www.redhat.com/archives/libvir-list/2013-June/msg00349.html v2: don't ignore priority and strip the trailing newline.
+typedef void (*udevLogFunctionPtr)(struct udev *udev, + int priority, + const char *file, + int line, + const char *fn, + const char *format, + va_list args); + +static void +ATTRIBUTE_FMT_PRINTF(6,0) +udevLogFunction(struct udev *udev ATTRIBUTE_UNUSED, + int priority, + const char *file, + int line, + const char *fn, + const char *fmt, + va_list args) { - VIR_ERROR_INT(VIR_LOG_FROM_LIBRARY, file, line, fn, fmt, args); + virBuffer buf = VIR_BUFFER_INITIALIZER; + const char *format = NULL; + + virBufferAdd(&buf, fmt, -1); + virBufferTrim(&buf, "\n", -1); +
We need a virBufferError checking here before formatting into a string. Others is good for me. ACK with checking added here. Guannan
On 14.06.2013 10:53, Guannan Ren wrote:
On 06/13/2013 09:56 PM, Ján Tomko wrote:
Call virLogVMessage instead of virLogMessage, since libudev called us with a va_list object, not a list of arguments.
Honor message priority and strip the trailing newline.
https://bugzilla.redhat.com/show_bug.cgi?id=969152 --- v1: https://www.redhat.com/archives/libvir-list/2013-June/msg00349.html v2: don't ignore priority and strip the trailing newline.
+typedef void (*udevLogFunctionPtr)(struct udev *udev, + int priority, + const char *file, + int line, + const char *fn, + const char *format, + va_list args); + +static void +ATTRIBUTE_FMT_PRINTF(6,0) +udevLogFunction(struct udev *udev ATTRIBUTE_UNUSED, + int priority, + const char *file, + int line, + const char *fn, + const char *fmt, + va_list args) { - VIR_ERROR_INT(VIR_LOG_FROM_LIBRARY, file, line, fn, fmt, args); + virBuffer buf = VIR_BUFFER_INITIALIZER; + const char *format = NULL; + + virBufferAdd(&buf, fmt, -1); + virBufferTrim(&buf, "\n", -1); +
We need a virBufferError checking here before formatting into a string.
In which case we want to got with fmt as provided. Michal
On 06/14/2013 11:18 AM, Michal Privoznik wrote:
On 14.06.2013 10:53, Guannan Ren wrote:
On 06/13/2013 09:56 PM, Ján Tomko wrote:
+ virBuffer buf = VIR_BUFFER_INITIALIZER; + const char *format = NULL; + + virBufferAdd(&buf, fmt, -1); + virBufferTrim(&buf, "\n", -1); +
We need a virBufferError checking here before formatting into a string.
virBufferContentAndReset already checks for the error and returns NULL if there was one. The only possible error here is ENOMEM.
In which case we want to got with fmt as provided.
Michal
If we aren't going to report the buffer error, I think checking format for NULL should be enough. I'll squash this in before pushing: diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index b462549..bb58415 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -374,10 +374,9 @@ udevLogFunction(struct udev *udev ATTRIBUTE_UNUSED, format = virBufferContentAndReset(&buf); - if (format) - virLogVMessage(VIR_LOG_FROM_LIBRARY, - virLogPriorityFromSyslog(priority), - file, line, fn, NULL, format, args); + virLogVMessage(VIR_LOG_FROM_LIBRARY, + virLogPriorityFromSyslog(priority), + file, line, fn, NULL, format ? format : fmt, args); VIR_FREE(format); } Jan
On 06/14/2013 06:54 AM, Ján Tomko wrote:
On 06/14/2013 11:18 AM, Michal Privoznik wrote:
On 14.06.2013 10:53, Guannan Ren wrote:
On 06/13/2013 09:56 PM, Ján Tomko wrote:
+ virBuffer buf = VIR_BUFFER_INITIALIZER; + const char *format = NULL; + + virBufferAdd(&buf, fmt, -1); + virBufferTrim(&buf, "\n", -1);
Coverity analysis uncovered a complaint in last nights run after this was pushed. The virBufferTrim() call has "other" places where the return status is checked, but this call doesn't. Using cscope I also see that virNetSSHCheckHostKey() in virnetsshsession.c does not check return status. FILE: src/node_device/node_device_udev.c Coverity error seen in the output: ERROR: CHECKED_RETURN FUNCTION: udevLogFunction Coverity Events: 372 virBufferAdd(&buf, fmt, -1); Calling function "virBufferTrim(virBufferPtr, (1) Event check_return: char const *, int)" without checking return value (as is done elsewhere 5 out of 6 times). No check of the return value of (7) Event unchecked_value: "virBufferTrim(&buf, "\n", - 1)". [example_checked][example_checked] Also see events: [example_checked][example_checked] [example_checked] ------------------------------------------------------------
+
We need a virBufferError checking here before formatting into a string.
virBufferContentAndReset already checks for the error and returns NULL if there was one. The only possible error here is ENOMEM.
In which case we want to got with fmt as provided.
Michal
If we aren't going to report the buffer error, I think checking format for NULL should be enough.
I'll squash this in before pushing:
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index b462549..bb58415 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -374,10 +374,9 @@ udevLogFunction(struct udev *udev ATTRIBUTE_UNUSED,
format = virBufferContentAndReset(&buf);
- if (format) - virLogVMessage(VIR_LOG_FROM_LIBRARY, - virLogPriorityFromSyslog(priority), - file, line, fn, NULL, format, args); + virLogVMessage(VIR_LOG_FROM_LIBRARY, + virLogPriorityFromSyslog(priority), + file, line, fn, NULL, format ? format : fmt, args);
VIR_FREE(format); }
Jan
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (4)
-
Guannan Ren -
John Ferlan -
Ján Tomko -
Michal Privoznik