On Wed, Jan 23, 2013 at 4:39 PM, Jakub Jelinek <ja...@redhat.com> wrote: > On Wed, Jan 23, 2013 at 04:24:01PM +0400, Evgeniy Stepanov wrote: >> > So, e.g. whenever match_spec >> > > returns 0, it should break out of the loop, rather than continue. >> > > And for %hh it doesn't check following letters, no match_spec at all. >> >> That's cause they don't change the write size. The same for %h, the >> following letters don't matter to us. It seems safe to "continue" as >> soon as we are sure we know what the current spec's write length is. > > You can't know. What if say %hhQ is seen where Q is a user spec defined > through some future hook? For printf you can already add things like > that, and say let the va_arg argument be a pointer to some struct > (some people e.g. print struct sockaddr* that way etc.). What if it takes > more than one pointer. IMHO you really need to parse the whole directive, > and only if you detect no errors, check the corresponding pointer (unless > %*), if you detect any errors or unknown things, just give up immediately.
What if these future hooks allow one to redefine existing specs? :) Fighting unknown future changes sounds hopeless. The only reliable way to handle them is keeping both implementations in sync, but that's probably not worth it. >> > > %[, %s, %c, %n, %ls, %lc, %p, %A and many others aren't handled FYI >> > > (again, not >> > > a problem if the implementation is conservative). Handling %c, %s and >> > > %[0123456789] style would be worthwhile though, I bet most of the buffer >> > > overflows are from the use of those scanf specifiers. >> >> At least some of those are handled (%p, %n). Not sure what %A is, it's >> not in my man page. > > http://pubs.opengroup.org/onlinepubs/9699919799/functions/fscanf.html > >> > > BTW, %as, %aS and %a[ should always result in giving up, as there is an >> > > ambiguity between GNU extensions and C99/POSIX and libasan can't know how >> > > glibc resolves that ambiguity. >> > > >> > > 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? > > If you mail me (or check in?) some fixes for the above mentioned issues, I > can do some testing. But right now I'm pretty sure it would just fail. > > Jakub
