Bug 11668

Summary: crashes on platforms where vsnprintf() violates POSIX and C99
Product: dbus Reporter: Peter O'Gorman, The Written Word, Inc. <pogma>
Component: coreAssignee: Simon McVittie <smcv>
Status: RESOLVED FIXED QA Contact: John (J5) Palmieri <johnp>
Severity: normal    
Priority: medium CC: cosimo.alfarano, hp, ralf.habacker, tj, will, zeuthen
Version: unspecifiedKeywords: patch
Hardware: Other   
OS: HP-UX   
URL: http://cgit.freedesktop.org/~smcv/dbus/log/?h=14-vsnprintf-11668
Whiteboard:
i915 platform: i915 features:
Bug Depends on:    
Bug Blocks: 36074    
Attachments: [PATCH 1/2] Cope with platforms whose vsnprintf violates both POSIX and C99
[PATCH 2/2] In Windows _dbus_printf_string_upper_bound, don't crash on malloc failure
replacement patch, now that Ralf has applied 2/2 and half of 1/2

Description Peter O'Gorman, The Written Word, Inc. 2007-07-20 09:02:31 UTC
While posix does require that vsnprintf return the number of characters that would have been put into the beffer less the trailing NULL byte, should the buffer have been big enough, not all systems do this.

At least HP-UX, IRIX and Tru64 Unix have non-conforming vsnprintf, the HP-UX implementation returns -1 if the buffer is too small, the Tru64 and IRIX implementations return the number of chars actually copied into the buffer less the trailing NULL. This causes dbus to seg fault with 'dbus-daemin --introspect' on those platforms.

Dbus can either use the gnulib implementation of vsnprintf, come up with its own implementation, or patch _dbus_printf_string_upper_bound.
Comment 1 Peter O'Gorman, The Written Word, Inc. 2011-01-13 10:12:06 UTC
*** Bug 16662 has been marked as a duplicate of this bug. ***
Comment 2 Simon McVittie 2011-04-07 10:37:30 UTC
Does vsnprintf still do this in current versions of these OSs?
Comment 3 Simon McVittie 2011-06-22 06:35:43 UTC
(In reply to comment #0)
> Dbus can either use the gnulib implementation of vsnprintf

It's LGPL, whereas D-Bus is under a bizarre dual-license which is unlikely to be changed (because one copyright holder is whoever received the assets of Codefactory AB when it folded), so no.

Something more permissively-licensed like Trio <http://daniel.haxx.se/projects/trio/> would be a possibility, though.

> come up with its own implementation

Life's too short, tbh.

> or patch _dbus_printf_string_upper_bound

It turns out the Windows implementation already has roughly the behaviour we want. Performance-wise, it's terrible (one malloc/free pair for each printf that doesn't fit in a buffer on the stack), but that's what you get for running on a pre-C99 libc.
Comment 4 Simon McVittie 2011-06-22 06:36:53 UTC
Created attachment 48283 [details] [review]
[PATCH 1/2] Cope with platforms whose vsnprintf violates both POSIX  and C99
Comment 5 Simon McVittie 2011-06-22 06:37:52 UTC
Created attachment 48284 [details] [review]
[PATCH 2/2] In Windows _dbus_printf_string_upper_bound, don't crash on malloc failure
Comment 6 Simon McVittie 2011-06-22 06:39:31 UTC
(Cc Ralf for review of a Windows bugfix.)
Comment 7 Simon McVittie 2011-06-22 06:40:46 UTC
(In reply to comment #5)
> Created an attachment (id=48284) [details]
> [PATCH 2/2] In Windows _dbus_printf_string_upper_bound, don't crash on malloc
> failure

This requires at least this part of patch 1:

--- a/dbus/dbus-string.c
+++ b/dbus/dbus-string.c
@@ -1255,6 +1255,9 @@ _dbus_string_append_printf_valist  (DBusString        *str
   /* Measure the message length without terminating nul */
   len = _dbus_printf_string_upper_bound (format, args);
 
+  if (len < 0)
+    return FALSE;
+
   if (!_dbus_string_lengthen (str, len))
     {
       /* don't leak the copy */
Comment 8 Ralf Habacker 2011-07-02 21:32:05 UTC
(In reply to comment #7)
> (In reply to comment #5)
> > Created an attachment (id=48284) [details] [details]
> > [PATCH 2/2] In Windows _dbus_printf_string_upper_bound, don't crash on malloc
> > failure
> 
> This requires at least this part of patch 1:
> 
> --- a/dbus/dbus-string.c
> +++ b/dbus/dbus-string.c
> @@ -1255,6 +1255,9 @@ _dbus_string_append_printf_valist  (DBusString       
> *str
>    /* Measure the message length without terminating nul */
>    len = _dbus_printf_string_upper_bound (format, args);
> 
> +  if (len < 0)
> +    return FALSE;
> +
>    if (!_dbus_string_lengthen (str, len))
>      {
>        /* don't leak the copy */

commited and pushed to git master
Comment 9 Ralf Habacker 2011-07-02 21:45:57 UTC
(In reply to comment #7)
> (In reply to comment #5)
> > Created an attachment (id=48284) [details] [details]
> > [PATCH 2/2] In Windows _dbus_printf_string_upper_bound, don't crash on malloc
> > failure
> 
> This requires at least this part of patch 1:

looks good - this patch and part 1 of the first patch committed
Comment 10 Simon McVittie 2011-07-27 06:18:58 UTC
Created attachment 49614 [details] [review]
replacement patch, now that Ralf has applied 2/2 and half of 1/2

This is the bit that still needs review. I believe it should work for all platforms mentioned in the autoconf documentation, except those where the length is ignored completely (64-bit Solaris 7, apparently). In particular:

* Tru64/IRIX return the truncated length on truncation
* HP-UX returns negative on truncation
Comment 11 Simon McVittie 2011-09-19 07:28:07 UTC
Anyone who still cares about Tru64, IRIX or HP-UX: please test this.

Anyone who cares about D-Bus being portable to ye olde proprietary Unix: please review this.
Comment 12 Will Thompson 2011-09-21 01:24:53 UTC
Review of attachment 49614 [details] [review]:

The great thing about standard APIs is that portability becomes so much easier. >_<

Assuming your breakdown of the different bugs in weird Unices is accurate, then I think this patch should work. The cost to Linux of this patch is the (unlikely) bufsize == len path making one redundant vsprintf call, and a couple of extra branches. Probably dwarfed by the cost of the original vsprintf call, I guess.
Comment 13 Simon McVittie 2011-09-21 03:40:58 UTC
Fixed in git for 1.4.16 and 1.5.8, thanks

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.