DevSolar / pdclib

The Public Domain C Library
https://pdclib.rootdirectory.de
Creative Commons Zero v1.0 Universal
228 stars 41 forks source link

scanf crashes #8

Closed JayFoxRox closed 4 years ago

JayFoxRox commented 4 years ago

This custom test case yields problems:

    int a, b, c;
    SCANF_TEST( 0, "foo 0", " %n%*s%n %n", &a, &b, &c );
    TESTCASE( a == 0 );
    TESTCASE( b == 3 );
    TESTCASE( c == 4 );

Here is some information from gdb:

Program received signal SIGSEGV, Segmentation fault.
0x00005555555645d3 in _PDCLIB_scan (spec=0x55555556732c "n", status=0x7fffffffe550) at pdclib/functions/_PDCLIB/_PDCLIB_scan.c:394
394             *val = status->i;
(gdb) print val
$1 = (int *) 0x0
(gdb) print *status
$2 = {base = -1, flags = 0, n = 1, i = 4, current = 0, s = 0x0, width = 18446744073709551615, prec = 0, stream = 0x55555556b620 <_PDCLIB_sin>, arg = {{gp_offset = 40, fp_offset = 48, overflow_arg_area = 0x7fffffffe6b0, reg_save_area = 0x7fffffffe5f0}}}

Downstream issue where this affects a real application: https://github.com/XboxDev/nxdk-pdclib/issues/16

JayFoxRox commented 4 years ago

Looks like missing suppression on %*c, %*s, %*[ and %*n (neither of which has a testcase yet).

Here is a (very hacky) test-case for %*s:

    memset( buffer, '\0', 100 );
    SCANF_TEST( 1, "foo", "%*s", buffer);
    TESTCASE( memcmp( buffer, "\0", 100 ) == 0 );

The code isn't checking for E_suppressed in this code:

https://github.com/DevSolar/pdclib/blob/4f5b4339a48890599d8fb85bfa2fc6f031d47fe6/functions/_PDCLIB/_PDCLIB_scan.c#L255-L257

this code:

https://github.com/DevSolar/pdclib/blob/4f5b4339a48890599d8fb85bfa2fc6f031d47fe6/functions/_PDCLIB/_PDCLIB_scan.c#L286-L288

this code:

https://github.com/DevSolar/pdclib/blob/4f5b4339a48890599d8fb85bfa2fc6f031d47fe6/functions/_PDCLIB/_PDCLIB_scan.c#L348-L349

and this code:

https://github.com/DevSolar/pdclib/blob/4f5b4339a48890599d8fb85bfa2fc6f031d47fe6/functions/_PDCLIB/_PDCLIB_scan.c#L391-L393

However, it is checked for other argument types:

https://github.com/DevSolar/pdclib/blob/4f5b4339a48890599d8fb85bfa2fc6f031d47fe6/functions/_PDCLIB/_PDCLIB_scan.c#L523-L524

DevSolar commented 4 years ago

Verified. Looking into it.

DevSolar commented 4 years ago

The bug itself is fixed in SVN. I found a couple of related issues I want to look into before I close this and push to GitHub.

JayFoxRox commented 4 years ago

The version in SVN does not seem to explicitly handle %*n; this C spec says this about n:

If the conversion specification includes an assignment-suppressing character or a field width, the behavior is undefined.

So technically the behaviour is right.

However, t think it might be a sane idea to report an error of some kind for %*n.

For a different opinion, this is what my linux man-page says:

n Nothing is expected; instead, the number of characters consumed thus far from the input is stored through the next pointer, which must be a pointer to int. This is not a conversion and does not increase the count returned by the function. *The assignment can be suppressed with the assignment-suppression character, but the effect on the return value is undefined*. Therefore %n conversions should not be used.

Which implies to me that users on linux would expect it to not consume a pointer using va_arg, however, the wording of the man-page is vague.

However, I'm personally not interested in %*n myself, so I don't really care if / how it is supported - I'm just leaving my thoughts in case someone else runs into this issue later.

DevSolar commented 4 years ago

Added "support" for %*n by way of, effectively, doing nothing. (This was indeed corollary to the %s / %c / %[ bug, I didn't think of E_suppressed in that context and forgot to cater for it.)

DevSolar commented 4 years ago

Fixes for these issues, plus one for the return code handling of %[ (which was broken as well), have been pushed to GitHub.

JayFoxRox commented 4 years ago

Thanks! Looks good (although I did not test the changes yet).

While reviewing the push to GitHub I noticed this typo: https://github.com/DevSolar/pdclib/commit/5d02542e6e1bda6525cda3acb4b12037e8764922#diff-b7d874035913ff9cf1baad83365fa334R620 (COMBINATIONi - note the i at the end)