getch() crashes with PDC_WIDE

[rhaberkorn]

In 4.5.1 (MSYS) the following crashes both in Wincon and WinGUI when pressing a multi-byte non-ANSI key:

#include <pdcurses.h>

int main(void)
{
        initscr();
        getch();
        endwin();
        return 0;
}

Compile with

cc -o getch-utf getch-utf.c -DPDC_WIDE -I/mingw64/include/pdcurses -lpdcurses_wincon -lgdi32 -lcomdlg32 -lwinmm

The assertion is

Assertion failed: count <= n - i, file ../pdcurses/util.c, line 492

This did not happen in 4.4.0, so it's a regression. I would expect to receive the individual bytes of the multi-byte sequence in several getch() calls (every byte as its own keypress event). This is how ncurses behaves as well.

I do not use the get_wch() class of functions as I'd like to stay compatible with non-widechar (n)curses builds for the time being - if that has any real-life significance is another question.

[Bill-Gray]

Hmmm... something must have been mis-built. That assertion is in the UTF-8 decoding routine, and you're building with a wide (but not forced UTF-8) library. At the very least, util.c must have been compiled with UTF8=Y, and initscr.c without it. Otherwise, you'd have gotten a failure to link when running the above command.

Perhaps make clean and re-build?

I build pretty much everything using UTF8=Y and would recommend not dealing with the steaming putrid mess of code pages and such. However, this library does get used with old projects, many of which will only work with 8-bit characters or with non-UTF8 wide characters. Those options will have to be maintained (which, fortunately, hasn't been all that big a deal).

However, I do see something that probably is a bug : in checking to see if we'd have a buffer overrun in that function, the trailing \0 isn't being counted. That is to say, if we added

    assert( i < n);

before dest[i] = '\0', we would find that it could be triggered (with i = n; don't think we can get i > n).

[Bill-Gray]

Should note that you're correct that the assert( count <= n - i) didn't exist in v4.4.0; it was added later, in commit 7d704dd. Before it was added, we would simply keep on decoding and quite possibly over-write the end of the buffer, either segfaulting or corrupting memory or (if we were quite lucky) getting away with it.

[rhaberkorn]

Hmmm... something must have been mis-built. That assertion is in the UTF-8 decoding routine, and you're building with a wide (but not forced UTF-8) library. At the very least, util.c must have been compiled with UTF8=Y, and initscr.c without it. Otherwise, you'd have gotten a failure to link when running the above command.

I am using the MSYS build. Linking of the toy example above works both with and without -DPDC_FORCE_UTF8. I assume that the MSYS builds are forced-UTF8.

[GitMensch]

For MSYS2 builds: those provide a wrapper header so best only `#include <pdcurses.h>" - no need for any defines then (other than possibly PDC_DLL_BUILD [on the command line] and PDC_NMOUSE [most likely in your source]).
And yes, those are UTF8-forced.

https://github.com/msys2/MINGW-packages/blob/478f1fedd7534f32c47ec28c4304e937e2a91407/mingw-w64-pdcurses/PKGBUILD#L111-L118

[GitMensch]

Note: the interesting part here is that this also happens on WinGUI!

[Bill-Gray]

My apologies; I jumped to a wrong conclusion here.

Testing your example code (modified version at bottom) shows that this is a general PDCursesMod thing, not Windows-specific. The assert() triggers on all platforms that I've tested. (Which, in a way, is a good thing. I can fix bugs in my own code. I can only work around bugs in Microsoft's code. But I digress.)

The bug in question is rather embarrassing. It can be fixed with the following patch :

--- a/pdcurses/getch.c
+++ b/pdcurses/getch.c
@@ -758,9 +758,11 @@ int wgetch(WINDOW *win)
-            wchar_t c = (wchar_t)rval;
+            wchar_t c[2];
 
-            n_buff = PDC_wcstombs( (char *)buffered, &c, 1);
+            c[0] = (wchar_t)rval;
+            c[1] = L'\0';
+            n_buff = PDC_wcstombs( (char *)buffered, c, sizeof( buffered));

getch() works by calling _raw_wgetch() to get wide characters, and then converts them to multi-byte form as they come in. Except it didn't do it correctly.

I think that when I wrote the original line, I thought the third argument to PDC_wcstombs() gave the number of wide characters to be converted to multi-byte form. It's actually the size of the buffer into which the multi-byte stream goes; the number of wide characters read is determined by the L'\0' terminator.

Before I added the assert() in PDC_wcstombs(), I got away with it cleanly; the wide character was converted to up to four bytes, but buffered[] was large enough to accommodate that. (It's actually eight bytes, just in case there's some exotic multi-byte coding out there.)

The only sign of trouble would have been that if you hit, say, ⅔, you would not receive the three characters e2, 85, 94 (proper expansion of that character into UTF8); you'd just get the e2. And similarly for any multi-byte sequence. And it appears nobody noticed that.

Will check a bit more, and we do still have the mouse mask issue (now firmly disassociated with this bug... and I've still failed to replicate it; my examples stubbornly insist on producing presses and releases).

For the sake of completeness, here's the slightly modified version of your getch-utf.c code that I used to show what bytes were actually returned.

#include "curses.h"

int main(void)
{
        int c;

        initscr();

        while( 'q' != (c = getch()))
            printw( " (%x) ", c);
        endwin();

        return 0;
}

[rhaberkorn]

The only sign of trouble would have been that if you hit, say, ⅔, you would not receive the three characters e2, 85, 94 (proper expansion of that character into UTF8); you'd just get the e2. And similarly for any multi-byte sequence. And it appears nobody noticed that.

I could definitely retrieve all bytes of non-ANSI characters in PDCursesMod v4.4.0.

[Bill-Gray]

I could definitely retrieve all bytes of non-ANSI characters in PDCursesMod v4.4.0.

Yup, you're right again. I passed 1 as the last argument to PDC_wcstombs(). Had the error-checking been done properly, only one byte should have been returned. But the previous code didn't do that check, so you got all the bytes back. After deciphering the first wide character, the buffer would be "full" or overfill, so the lack of a null terminator didn't lead to trouble.

So... the code actually worked perfectly, despite being abysmally mis-written. It's honestly pretty impressive that it could be so totally wrong, but still run without errors.

[rhaberkorn]

Thanks for fixing it! I suppose it's going to be quite some time until this finds its way into another MSYS package release? Thinking about working around this by using wget_wch().

[GitMensch]

I'm actually quite positive that there will be a new version soon, that will also allow to drop the -std=c17 options currently necessary.
I'd expect an updated MSYS2 package within two days afterwards.

[Bill-Gray]

I'm actually quite positive that there will be a new version soon, that will also allow to drop the -std=c17 options currently necessary.

I think we're ready to go, with the minor caveat that it'd be nice to also have a fix for issue #255 (i.e., if you #define PDC_NCMOUSE, you should get an ncurses-compatible mouse interface, complete with its shortcomings). I'm hoping to get time to puzzle that out tonight (i.e., next few hours).

But the current code fixes both the getch() crashes (this issue) and the -std=c17 issue (that's issue #333). If I'm not utterly convinced that issue #255 is fixed, we'll go with what is currently posted, just nudging up PDC_VER_DATE, PDC_VER_CHANGE, and modifying the top line of HISTORY.md accordingly.

(Which means that if anyone sees reason(s) a new release should not occur quite yet, please speak now or forever hold your peace. Whenever I hit that "publish release" button, I'm always slightly nervous that I've overlooked something...)

[Bill-Gray]

Having heard no objections, I've hit the Big Red Button for release 4.5.2.

Just to give you some ideas as to what may happen for 4.5.3 (or 4.6), if I've time to do it :

• This release does not fix issue mousemask, mouse wheel - different behave than ncurses #255. I thought that was going to be an easy one, and still think it will be, once I come to a solid conclusion of what the fix will be.

• I'll probably attempt to reformat the 'portability' tables, a la wmcbrine/PDCurses@d75af9836e62a8852520a. (I used to work a bit to keep this fork in sync with William's version; it made it easier to check, for example, if an issue was specific to PDCursesMod. We're pretty thoroughly forked at this point, though. But this is a relatively easy bit to keep synched.)

• We've a few relatively simple issues to fix (Makefiles: use of CPPFLAGS #334, for example, and possibly Makefile overhaul, moving PREFIX to CROSS_COMPILE, providing PREFIX and DESTDIR #336).

• We may need to re-visit SDL3 (issue Do we need an SDL3 port? #271). I'm not sure, but I'm getting the impression the differences between SDL3 and SDL2 will at least require the occasional #ifdef. (We still have way more code duplication between SDL1, SDL2, and GL than we should, which is a somewhat different but related issue.)

• And probably of interest only to one person : I'd like to have mouse events in the framebuffer/DRM platform, and we're about 90% of the way there.

  I've no idea if anybody except me is using that platform. I think it's pretty awesome to be able to support Linux and *BSD systems that don't have X11 running on them, though.