PDCurses issue in getch.c function _paste() when PDC_WIDE is defined

[nhmall]

ERROR: AddressSanitizer: heap-buffer-overflow on address 0x06308df8 at pc 0x00e21cfc bp 0x0269ef18 sp 0x0269ef0c
WRITE of size 2 at 0x06308df8 thread T0
    #0 0xe21cfb in PDC_mbstowcs C:\Users\test\pdcursesmod\pdcurses\util.c:457
    #1 0xe1653a in _paste C:\Users\test\pdcursesmod\pdcurses\getch.c:282
    #2 0xe16d57 in _raw_wgetch_no_surrogate_pairs C:\Users\test\pdcursesmod\pdcurses\getch.c:569
    #3 0xe167de in _raw_wgetch C:\Users\test\pdcursesmod\pdcurses\getch.c:637
    #4 0xe17745 in wgetch C:\Users\test\pdcursesmod\pdcurses\getch.c:740

wincon
PDC_WIDE is defined.

getch.c function _paste() line 276 PDC_getclipboard(&paste, &len) sets the value of len (via PDC_getclipboard()'s call to wcslen). It does not include the terminating null in that value.

The malloc at line 281 does not allocate any space for the trailing null.

getch.c function _paste() line 282 passes the buffer address and that value of len to util.c function PDC_mbstowcs(wpaste, past, len).

In util.c function PDC_mbstowcs, where the value passed from the caller's len is represented by argument n, the following line 455 is encountered.

455: size_t i = mbstowcs(dest, src, n);

The mbstowcs() function returns the number of wide characters
that make up the converted part of the wide-character string, not
including the terminating null wide character.

The value of i and n can match at this point under the circumstances described, and when they do the next line of util.c function PDC_mbstowcs() line 457 writes past the end of the allocated buffer.

457: dest[i] = '\0';

Changing lines 281 and 282 of getch.c function _paste() as follows avoids the write past the end of the buffer.

280: #ifdef PDC_WIDE
281:    wpaste = (wchar_t *)malloc((len + 1) * sizeof(wchar_t));
282:    len = (long)PDC_mbstowcs(wpaste, paste, len + 1);

The situation could be detected in util.c function PDC_mbstowcs() by adding a test for the index i being greater than or equal to the buffer size n above line 457, but it is really too late by then because neither option available for avoiding the write past the end of the buffer would be desirable:

leaving an unterminated string

if (i < n) dest[i] = '\0';

or silent truncation:

if (i >= n) i = n - 1;
dest[i] = '\0';

[Bill-Gray]

(Forehead slap) You are correct. Thank you.

I verified it with Valgrind just to be completely sure, and it reported the overwriting by four bytes (size of a wide character in Linux) at the end of the array. As expected, using len + 1 in the appropriate locations caused the problem to go away.

@wmcbrine may be interested, since the same bug affects PDCurses.

[GitMensch]

The issue that is still open: those functions are not tested with the existing test programs, otherwise this would have come up before with valgrind.
Would there be an option to "automatically" paste? Otherwise there may should be an output like "press paste key now" in some of the tests.

[GitMensch]

ping @Bill-Gray Do you see any option to execute the paste function in a (semi-)automated test to catch possible issues in the future without doing those tests completely manually?

[Bill-Gray]

Um. I could imagine putting text into the clipboard, then doing something along the lines of ungetch( 22); (Ctrl-V = ASCII 22) to get it to be pasted. (With added trickery on platforms where it should be Ctrl-Shift-V.) However, it would be a low priority. At present, we have the clipboard test routines in testcurs.

I suppose we could test for this specific problem, except that we've fixed it, and a test specific to this bug would not necessarily address whatever bugs may remain.

[GitMensch]

If nothing else, then it would make "more sure" to not revamp this bug later; but I is fine to say "try to run valgrind through completely testcurs manually" (actually automating this would be quite useful).

But I'm fin with whatever you go for :-)