[RP] Re: Bug#423141: more questions about #423141

[Bernhard R. Link]

I'm CCing ratpoison's mailinglist, so people there can look at this, too.
For those on the list: the history of this thread can be found at
http://bugs.debian.org/423141

* H. S. Teoh <address@hidden> [070814 06:13]:
> Now that I know where the bug is, I've managed to reproduce it with a
> debug build of ratpoison. Using gdb backtrace, I've located the
> problematic code: the xvsprintf() function in main.c. Towards the end of
> this function, there is this bit of code:
> 
> -------------------------------------------------------------
>       nchars = vsnprintf (buffer, size, fmt, ap_copy);
> #if defined(va_copy) || defined(__va_copy)
>       va_end (ap_copy);
> #endif
> 
>       if (nchars > -1 && nchars < size)
>         return buffer;
>       else if (nchars > -1)
>         size = nchars + 1;
>       else
>         size *= 2;
> -------------------------------------------------------------
> 
> The last else clause is blatantly wrong: if vsnprintf() returns a
> negative value (according to the manpage, this occurs if there is an
> output error), then this code will double the size and try to format it
> again.  Since the problem is not caused by the buffer size, but by an
> output error (which I suppose is caused by the presence of characters in
> the output which for some reason is in the wrong encoding), doubling the
> buffer size solves nothing at all. So it just loops until size is larger
> than the amount of available RAM, and then the allocator calls abort()
> and dies.

I'm guessing the reason for this is history (from vsprintf manpage):
| NOTES
|   The glibc implementation of the functions snprintf() and vsnprintf()
|   conforms to the C99 standard, i.e., behaves as described above, since
|   glibc version 2.1. Until glibc 2.0.6 they would return -1 when the output
|   was truncated.

> I'd like to confirm my analysis with an actual code fix, but I'm not
> sure exactly what to do if vsnprintf() returns a negative value. The
> only clean way I know of is to use wide-character formatting functions,
> but it's non-trivial to do this with this code, and I'm not even sure if
> ratpoison is setting the locale properly in the first place. Maybe when
> I get more time I'll take another look at this.

I guess there are three issues here:

1) I'd guess it is better to saveguard this and add some maximum number
   of characters to test for (at least with negative return values).
   This would help against other possible future problems causing -1
   returns.

2) It might be some problem with giving invalid utf-8 characters into
   this function. While searching for the reason of this bug I found
   some problems when both ratpoison and the clients use a UTF-8
   locale (though that causes only misdisplays for me).
   It's in cvs since July 19th. (I also though I uploaded a Debian
   package with a backport of that patch, but I seem to have forgotten
   it).

3) finally vsnprinf might indeed the wrong function for that. I've not
   yet looked into that. But looking at the manpage I'd guess that that
   should only generate displaying problems (as the number means bytes
   and not characters). I guess this needs someone knowing this issues
   to look into it.

> In the meantime, I hope this info may help you find a fix.

Thanks a lot for tracking this down.

Hochachtungsvoll,
        Bernhard R. Link