Fix heap buffer underflow in PDC_mbstowcs() and PDC_wcstombs()

[serhiy-storchaka]

Fixes #383.

mbstowcs() / wcstombs() return (size_t)-1 on an invalid multibyte sequence or a wide character the current locale cannot encode. PDC_mbstowcs() and PDC_wcstombs() stored the terminating NUL at dest[i] without checking, so dest[(size_t)-1] wrote one element before the start of the buffer — an out-of-bounds write corrupting adjacent memory. This is reachable from winnstr() / winnwstr() on a window that holds a character the current locale cannot encode (details and a reproducer in #383).

Guard the (size_t)-1 error return before writing the terminator. On failure the functions now yield an empty, properly terminated string — the same result as "nothing was convertible".

Only the non-PDC_FORCE_UTF8 branches were affected; the forced-UTF-8 paths already return early on error and never reach the terminator write.

[GitMensch]

@Bill-Gray we need to define how those functions behave when returning -1 (should the dest buffer be zero-terminated or not) and do it identical in both functions and w/wo PDC_FORCE_UTF8

[GitMensch]

per code above that is wrong - instead -1 should be returned

[GitMensch]

@Bill-Gray we should possibly return -1 here instead of asserting (then do the same for the -1 case below)
Without checking the code I'm not sure where we copy the trailing zero - and if we do that on each memcpy (which would be a waste)
Side note: While the compiler will likely optimize that, I'd suggest to const size_t m = m - 1.

[Bill-Gray]

You're both right, and this code is wrong no matter how you slice it. Pinging @wmbrine because the same issue is in "upstream" PDCurses and should be fixed there as well.

Quoting from the documentation at the top of the file,

   If the library is built for "forced" UTF8 encoding,
   the PDC_* functions do UTF8 encoding and decoding.  If it is built
   without forced encoding,  then the standard library functions are
   used instead.

The man page for wcstombs() says that (size_t)-1 is returned if a malformed sequence is encountered. It doesn't exactly say, but it looks to me as if the function just stops right there and doesn't append a '\0' to whatever has already been decoded. And this small bit of test code confirms that.

So. In the non-PDC_FORCE_UTF8 case, we should just set size_t i = wcstombs( dest, src, n); and not add a terminator. If the function succeeded, it already did that for us; if it didn't, no terminator should be added anywhere. dest[i] = '\0'; should be moved to above the #else (i.e., only used when UTF8 is forced).

Strictly speaking, we aren't quite done yet. The PDC_FORCE_UTF8 code should similarly return -1 if it encounters an invalid sequence, and should only add the '\0' if there's room in the buffer. You put that together with the above, and you get the following somewhat but not completely tested patch (works and passes "eyeball review", but the code is now complicated enough that I wanna put some test code together for it).

 diff --git a/pdcurses/util.c b/pdcurses/util.c
index 3c059f38..89fd014f 100644
--- a/pdcurses/util.c
+++ b/pdcurses/util.c
@@ -473,30 +473,33 @@ size_t PDC_mbstowcs(wchar_t *dest, const char *src, size_t n)
 size_t PDC_wcstombs(char *dest, const wchar_t *src, size_t n)
 {
 # ifdef PDC_FORCE_UTF8
-    size_t i = 0;
+    size_t i = 0, count = 1;
 
     assert( src);
     assert( dest);
     if (!src || !dest)
         return 0;
 
-    while( i + 4 < n && *src)
-       i += PDC_wc_to_utf8( dest + i, *src++);
-    while( i < n && *src)
+    while( count && i + 4 < n && *src)
+       i += (count = PDC_wc_to_utf8( dest + i, *src++));
+    while( count && i < n && *src)
     {
        char tbuff[4];
-       size_t count = (size_t)PDC_wc_to_utf8( tbuff, *src++);
 
-       assert( count <= n - i);  /* partial UTF-8 decoding indicates error */
+       count = (size_t)PDC_wc_to_utf8( tbuff, *src++);
+       assert( count <= n - i);    /* don't go past end of buffer */
        if( count > n - i)
            count = n - i;
        memcpy( dest + i, tbuff, count);
        i += count;
     }
+    if( !count)                 /* invalid UTF-8 sequence encountered */
+        return (size_t)-1;
+    if( i < n)
+        dest[i] = '\0';
 # else
     size_t i = wcstombs(dest, src, n);
 # endif
-    dest[i] = '\0';
     return i;
 }
 #endif

[serhiy-storchaka]

Thanks @Bill-Gray — agreed the original is wrong. But dropping the terminator entirely in the non-PDC_FORCE_UTF8 branch breaks winnstr(): wcstombs() only appends its own '\0' when the output is shorter than n, and winnstr() passes n = the cell count, so a full read is exactly n bytes with no terminator. Callers expect an n+1 buffer NUL-terminated, so it still has to be written on success.

I hit this via the CPython curses port: after the patch, addstr("abc"); instr(0, 0, 3) returned b'abc' plus ~250 bytes of trailing heap garbage.

So I kept the error contract only for the failure path — return (size_t)-1 immediately (leaving dest unterminated) on an unconvertible character, terminate at dest[i] only on success — and applied it symmetrically to PDC_mbstowcs() (per @GitMensch). Pushed to the branch.