On Mon, Feb 11, 2013 at 12:38:00PM +0100, Jakub Jelinek wrote:
> On Wed, Jan 23, 2013 at 04:24:01PM +0400, Evgeniy Stepanov wrote:
> > > > What if glibc adds a scanf hook (like it has already printf hooks), apps
> > > > could then register their own stuff and the above would then break. It
> > > > really should be very conservative, and should be checked e.g. with all
> > > > glibc's *scanf tests (e.g. stdio-common/scanf[0-9]*.c,
> > > > stdio-common/tst-sscanf.c).
> >
> > I'll add support for the missing %specs. About the testing, these
> > files seem like GPL, so I'd prefer not to look at them at all. We've
> > got a smallish test for the scanf implementation in sanitizer_common,
> > but it would be really great to run it on full glibc scanf tests.
> > Would you be willing to setup such testing gnu-side?
>
> Seems the code in llvm repo looks much better now to me than it used to,
> still it breaks on:
Jakub,
So there is likely to be at least one more remerge in libsanitizer for the
gcc 4.8 release? I saw that Richard felt that PR56128 justified one. FYI,
I'm interested because it would get us the new allocator support on darwin.
Jack
>
> #define _GNU_SOURCE 1
> #include <stdio.h>
> #include <string.h>
>
> int
> main ()
> {
> char *p;
> long double q;
> if (sscanf ("abcdefghijklmno 1.0", "%ms %Lf", &p, &q) != 2
> || strcmp (p, "abcdefghijklmno") != 0
> || q != 1.0L)
> return 1;
> return 0;
> }
>
> because it mishandles %ms.
>
> Attached patch fixes that and also handles %a when possible.
> There are cases where it is unambiguous, e.g.
> s%Las is s %La s, reading long double, or %ar is %a r, reading
> float, other cases where we can check at least something and keep checking
> the rest of the format string, e.g. for:
> %as
> we know it will either store float, or char * pointer, so just assume
> conservatively the smaller size, and other cases where we have to punt,
> %a[a%d]
> (where % appears in the [...] list).
>
> I've tested various glibc *scanf testcases with the GCC libsanitizer
> + enabling of # define SANITIZER_INTERCEPT_SCANF SI_NOT_WINDOWS
> + llvm svn diff between last merge point to GCC and current llvm trunk
> + this patch, and it looked good.
>
> There is one further issue, I'd say you need to pass down to scanf_common
> the result from the intercepted call, and use it to see if it is valid
> to call strlen on the ptr, or if just 1 should be assumed for SSS_STRLEN.
> Note 'n' doesn't count towards that. Because, it is unsafe to call strlen
> on arguments that haven't been assigned yet. Testcase that still fails:
>
> #define _GNU_SOURCE 1
> #include <stdio.h>
> #include <string.h>
>
> int
> main ()
> {
> int a, b, c, d;
> char e[3], f[10];
> memset (e, ' ', sizeof e);
> memset (f, ' ', sizeof f);
> if (sscanf ("1 2 a", "%d%n%n%d %s %s", &a, &b, &c, &d, e, f) != 3
> || a != 1
> || b != 1
> || c != 1
> || d != 2
> || strcmp (e, "a") != 0)
> return 1;
> return 0;
> }
>
> Oh, one more thing, on Linux for recent enough glibc's it would be
> desirable to also intercept:
> __isoc99_sscanf, __isoc99_scanf, __isoc99_vsscanf, __isoc99_fscanf,
> __isoc99_vfscanf, __isoc99_vscanf
> functions (guard the interception with
> #if defined __GLIBC_PREREQ && __GLIBC_PREREQ (2, 7)
> ). All these work exactly like the corresponding non-__isoc99_
> functions, just %a followed by s, S or [ always writes float, thus
> there is no ambiguity.
>
> Jakub
> --- sanitizer_common_interceptors_scanf.inc.jj 2013-02-08
> 13:21:51.000000000 +0100
> +++ sanitizer_common_interceptors_scanf.inc 2013-02-11 12:15:27.575871424
> +0100
> @@ -18,11 +18,12 @@
>
> struct ScanfDirective {
> int argIdx; // argument index, or -1 of not specified ("%n$")
> - bool suppressed; // suppress assignment ("*")
> int fieldWidth;
> + bool suppressed; // suppress assignment ("*")
> bool allocate; // allocate space ("m")
> char lengthModifier[2];
> char convSpecifier;
> + bool maybeGnuMalloc;
> };
>
> static const char *parse_number(const char *p, int *out) {
> @@ -119,6 +120,31 @@ static const char *scanf_parse_next(cons
> // Consume the closing ']'.
> ++p;
> }
> + // This is unfortunately ambiguous between old GNU extension
> + // of %as, %aS and %a[...] and newer POSIX %a followed by
> + // letters s, S or [.
> + if (dir->convSpecifier == 'a' && !dir->lengthModifier[0]) {
> + if (*p == 's' || *p == 'S') {
> + dir->maybeGnuMalloc = true;
> + ++p;
> + } else if (*p == '[') {
> + // Watch for %a[h-j%d], if % appears in the
> + // [...] range, then we need to give up, we don't know
> + // if scanf will parse it as POSIX %a [h-j %d ] or
> + // GNU allocation of string with range dh-j plus %.
> + const char *q = p + 1;
> + if (*q == '^')
> + ++q;
> + if (*q == ']')
> + ++q;
> + while (*q && *q != ']' && *q != '%')
> + ++q;
> + if (*q == 0 || *q == '%')
> + return 0;
> + p = q + 1; // Consume the closing ']'.
> + dir->maybeGnuMalloc = true;
> + }
> + }
> break;
> }
> return p;
> @@ -131,9 +157,7 @@ static bool scanf_is_integer_conv(char c
>
> // Returns true if the character is an floating point conversion specifier.
> static bool scanf_is_float_conv(char c) {
> - return char_is_one_of(c, "AeEfFgG");
> - // NOTE: c == 'a' is ambiguous between POSIX and GNU and, therefore,
> - // unsupported.
> + return char_is_one_of(c, "aAeEfFgG");
> }
>
> // Returns string output character size for string-like conversions,
> @@ -168,6 +192,21 @@ enum ScanfStoreSize {
> // Returns the store size of a scanf directive (if >0), or a value of
> // ScanfStoreSize.
> static int scanf_get_store_size(ScanfDirective *dir) {
> + if (dir->allocate) {
> + if (!char_is_one_of(dir->convSpecifier, "cCsS["))
> + return SSS_INVALID;
> + return sizeof(char *);
> + }
> +
> + if (dir->maybeGnuMalloc) {
> + if (dir->convSpecifier != 'a' || dir->lengthModifier[0])
> + return SSS_INVALID;
> + // This is ambiguous, so check the smaller size of char * (if it is
> + // a GNU extension of %as, %aS or %a[...]) and float (if it is
> + // POSIX %a followed by s, S or [ letters).
> + return sizeof(char *) < sizeof(float) ? sizeof(char *) : sizeof(float);
> + }
> +
> if (scanf_is_integer_conv(dir->convSpecifier)) {
> switch (dir->lengthModifier[0]) {
> case 'h':
> @@ -256,10 +295,6 @@ static void scanf_common(void *ctx, cons
> }
> if (dir.suppressed)
> continue;
> - if (dir.allocate) {
> - // Unsupported;
> - continue;
> - }
>
> int size = scanf_get_store_size(&dir);
> if (size == SSS_INVALID)
