Fix heap buffer underflow in PDC_mbstowcs()

[serhiy-storchaka]

Fixes #383.

mbstowcs() returns (size_t)-1 on an invalid multibyte sequence, without writing a terminator, and PDC_mbstowcs() then stored the terminator at dest[(size_t)-1], one element before the buffer. Return (size_t)-1 with an empty dest instead, as PDC_wcstombs() does since ea9baa4.

With the vt port and a guard element placed before the buffer, PDC_mbstowcs(buf, "AB\xff", 8) in a UTF-8 locale overwrote the guard with 0 and left buf unterminated; it now leaves the guard alone and sets buf[0] = 0.

The PDC_wcstombs() half of the original PR is superseded by ea9baa4 (see #390 / #391 for the exact-fit case).

[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.

[Bill-Gray]

Returning to this after quite a while...

My initial thought was that PDC_wcstombs() ought to match the native behavior of wcstombs() and simply stop when it hits an un-encodeable character. If it were exported from the PDCursesMod library, I might want it to do that.

However, it's internal to PDCursesMod, called in three places, and in each of them, we ought to have a guarantee of null termination if we get to the end of the buffer or hit an un-encodeable character.

I considered trying to return a partial string in such cases ("we got this far and then either hit an un-encodeable character or ran out of buffer space"). The problem is that wcstombs() provides no information in such cases as to how far it got. So I just set *dest = '\0'; in such cases.

Re "callers expect an n+1 buffer NUL-terminated" : sort of true, but callers really should check the return value. If it's ERR, it'll only be null-terminated as of this patch. With an older library, it may not be null-terminated. (Much as anyone calling wcstombs() has to check the return value, because the string won't be null-terminated if you ran out of buffer space or had an un-encodeable wide character.)

[serhiy-storchaka]

The *dest = '\0' for the out-of-space case is right in intent, but i >= n cannot tell "ran out of space" from "filled the buffer exactly": wcstombs() returns n in both cases. The exact fit is the common case for winnstr() and wgetnstr(), which convert n characters into n bytes, so since ea9baa4 every full read comes back as an empty string, and aborts on the assert in a build with assertions. Details in #390, fix in #391.

The null-termination guarantee does not need that branch: every caller gives PDC_wcstombs() a buffer of n + 1 bytes -- winnstr()/wgetnstr() by the documented contract ("be sure your buffer has room for that"), slk_label()'s temp[MAX_LABEL_LENGTH + 1], the clipboard's malloc(len + 1) -- so dest[n] = '\0' is always in bounds, as it was before. Only the (size_t)-1 failure needs *dest = '\0'.

I have rebased this PR onto master and reduced it to the PDC_mbstowcs() half, which still has the underflow.