Sanitize printf functions

[kcc]

Tests, please.
We don't have a framework to run the same test under asan, tsan and msan,
so I guess we may have a full test in asan/lit_tests/TestCases/ and two tiny tests in
msan/lit_tests/ and tsan/lit_tests/

(FYI: I don't promise continued quick reviews until ~ Jan 10)

asan/asan_intercepted_functions.h

79 ↗

(On Diff #6284)

Why scanf?

asan/asan_interceptors.cc

678 ↗

(On Diff #6284)

yes, please

sanitizer_common/sanitizer_common_interceptors_scanf.inc

377

This interceptor will cause many new bugs to be discovered
and the deployment in many cases will take time.
Please add a flag "check_printf" to sanitizer_common/sanitizer_flags.h (true by default)
and if this flag is false, don't do any checking.

[kcc]

also, please CC llvm-commits

[ygribov]

Tests, please.

Yup, I forgot to do `svn add' when generating diff. Will do.

We don't have a framework to run the same test under asan, tsan and msan

Note that current version only intercepts printf for asan (interceptors are implemented in asan_interceptors.cc). I don't have much experience with msan so I'll need your advice on how to proceed there.

FYI I don't promise continued quick reviews until ~ Jan 10

Same here.

asan/asan_intercepted_functions.h

79 ↗

(On Diff #6284)

I felt cynical and simply grabbed SANITIZER_INTERCEPT_SCANF from sanitizer_common/sanitizer_platform_interceptors.h in hope that printf and scanf always go together. What about renaming SANITIZER_INTERCEPT_SCANF to SANITIZER_INTERCEPT_FORMAT?

asan/asan_interceptors.cc

678 ↗

(On Diff #6284)

I tried this initially but realized that Msan already intercepts printf stuff. What's the general policy about what goes into sanitizer_common (e.g. strcmp) and what gets re-implemented in each sanitizer (e.g. memset)?

sanitizer_common/sanitizer_common_interceptors_scanf.inc

377

Makes sense, will do.

[ygribov]

also, please CC llvm-commits

Done!

[kcc]

asan/asan_intercepted_functions.h

79 ↗

(On Diff #6284)

I would have a separate SANITIZER_INTERCEPT_PRINTF

asan/asan_interceptors.cc

678 ↗

(On Diff #6284)

msan interceptors are rudimentary and will have to be replaced with these *eventually*.
Here is a suggestion: move the code to sanitizer_common_interceptors.inc
but define SANITIZER_INTERCEPT_PRINTF to 1 only in asan.
tsan and msan will follow later.

[eugenis]

asan/asan_interceptors.cc

678 ↗

(On Diff #6284)

We can enable it in MSan from the start (killing the old msan interceptors), just make sure that the new interceptors emulate a WRITE to the target buffer (for sprintf and friends).

[ygribov]

asan/asan_interceptors.cc

678 ↗

(On Diff #6284)

Is current emulation fine or do you want to check each specifier separately?

[eugenis]

asan/asan_interceptors.cc

678 ↗

(On Diff #6284)

I mean, when this is moved to common interceptors, you'll need to do COMMON_INTERCEPTOR_WRITE_RANGE() for the sprintf target buffer, like the current msan interceptors do.
That is in addition to reads and writes that you do already.

[ygribov]

Updates based on prev review.

Missing stuff:

• MSan still using old interceptors
• no tests for TSan and MSan

Plus a bunch of questions inlined (see TODO markers).

[ygribov]

@Evgeny: let me know if this looks sane to you wrt MSan.

[kcc]

Let others look at this change too.
We won't commit it before the end of our holidays (~Jan 10) anyway.

Until we don't have tsan tests, I suggest to disable this for tsan.

msan/msan_interceptors.cc

1294

we can do it as a separate commit (I'd even prefer that)

sanitizer_common/sanitizer_common_interceptors.inc

33

can you avoid this?

sanitizer_common/sanitizer_common_interceptors_scanf.inc

281

good idea. feel free to implement or to leave the TODO.
Actually this code base uses FIXME (w/o name) more often.

[ygribov]

Let others look at this change too.
We won't commit it before the end of our holidays (~Jan 10) anyway.

Sure, there's no push from my side.

Until we don't have tsan tests, I suggest to disable this for tsan.

k

msan/msan_interceptors.cc

1294

Sure.

sanitizer_common/sanitizer_common_interceptors.inc

33

Well, I need size_t for snprintf.

sanitizer_common/sanitizer_common_interceptors_scanf.inc

281

Actually this code base uses FIXME (w/o name) more often.

Got it. Anyway, this was more a question for reviewers than real TODO. I'll add format check in next rev.

[kcc]

sanitizer_common/sanitizer_common_interceptors.inc

33

please use SIZE_T and don't include any additional system headers

[ygribov]

Until we don't have tsan tests, I suggest to disable this for tsan.

Will making a copy of printf-1.c for TSan be enough? Or you want a test with an actual race?

sanitizer_common/sanitizer_common_interceptors.inc

33

Agreed, my bad.

[glider]

sanitizer_common/sanitizer_common_interceptors_scanf.inc

10

Please update this comment.
Also, how about renaming the file to something like "sanitizer_common_interceptors_printf_scanf.inc" or "sanitizer_common_interceptors_format.inc"?

281

We sure need to check |format|. Since you've already touched scanf_common in this CL, why not just add

COMMON_INTERCEPTOR_READ_RANGE(ctx, format, internal_strlen(format) + 1);

?

422

Two spaces before the comment, please.

sanitizer_common/sanitizer_platform_interceptors.h

75–76

I wonder why the lines above have this strange indentation, but let us fix that in a separate commit.

[ygribov]

sanitizer_common/sanitizer_common_interceptors_scanf.inc

10

Please update this comment.

Will do.

Also, how about renaming the file to something like "sanitizer_common_interceptors_printf_scanf.inc"

But won't this make diff unreadable? Perhaps this should be a separate patch?

281

Yup, already added this in new version of the patch. I didn't resubmit it because I wanted to gather reviews from all team members.

The bigger question is whether we want to check size of input buffer in sscanf and friends.

BTW what's the general rule: should I resubmit patch as often as possible or wait till everyone comments?

422

Do you think we need a warning here? If yes, I'll add it, if not - I'll remove the comment alltogether.

sanitizer_common/sanitizer_platform_interceptors.h

75–76

Looks like something lint should be able to handle.

[glider]

sanitizer_common/sanitizer_common_interceptors_scanf.inc

10

But won't this make diff unreadable? Perhaps this should be a separate patch?

SGTM

281

BTW what's the general rule: should I resubmit patch as often as possible or wait till everyone comments?

I think it's better to resubmit once you've addressed a round of comments.
Most of the team is on holidays now, so you'll never know whether everyone is going to comment.

281

The bigger question is whether we want to check size of input buffer in sscanf and friends.

Yes, that's nice to have, too.

422

We can't get an invalid size from format_get_value_size(), so it's ok to have a CHECK here.

sanitizer_common/sanitizer_platform_interceptors.h

75–76

I've removed these. There's no clear policy about these spaces anyway.

[ygribov]

sanitizer_common/sanitizer_common_interceptors_scanf.inc

281

Any idea how to achieve this? Scanfs won't tell us number of bytes read and I can't make use of %n in interceptor...

422

Will do.

[ygribov]

Updated based on comments by glider and kcc.

[ygribov]

Will be in standby till Jan 9.

[glider]

Looks mostly good.

asan/lit_tests/TestCases/printf-4.c

6

Please sort the includes alphabetically (here and in other tests)

sanitizer_common/sanitizer_common_interceptors_scanf.inc

11

How about this line? Is there a printf specification we're following?

323

Please swap this line with the above declaration and remove the empty line below.

337

How does this comment relate to the above one?

350

Good point, probably worth doing.

[ygribov]

asan/lit_tests/TestCases/printf-4.c

6

Will do.

sanitizer_common/sanitizer_common_interceptors_scanf.inc

11

Oh, this is a good one. I've blindly reused scanf parser but it seems that printf format has richer syntax: it has flags and precision + asterisk semantics is different. I'll fix this in next rev.

323

+1

337

Sorry, not quite clear. I believe we don't want to bother with positional args neither in scanf, nor in printf.

350

Ok, I can do this. Note that there are actually many more checks of this sort: intersection of format with target string, intersection of format with %n destination, intersection of format with scanf argument, etc., etc. I guess errors like this should already be handled by things like FORTIFY_SOURCE pretty well.

[eugenis]

sanitizer_common/sanitizer_common_interceptors.inc

698

I'd remove check_printf from this condition, otherwise we'd get an MSan regression when check_printf is disabled. Also, these writes don't depend on format string parsing, and are most certainly correct.

The same goes to other v*printf interceptors.

sanitizer_common/sanitizer_common_interceptors_scanf.inc

11

Yes, this is kind of scary.
AFAIK, glibc has a comprehensive set of tests for scanf format string parsing. Jakub Jelinek ran this code on them and found several tricky bugs in the parser. Could you look into doing the same for printf?

[ygribov]

sanitizer_common/sanitizer_common_interceptors.inc

698

Note that check_printf is true by default and MSan will still use it's own interceptors for now.

sanitizer_common/sanitizer_common_interceptors_scanf.inc

11

AFAIK, glibc has a comprehensive set of tests for scanf format string parsing.

Are we talking about these:

$ find glibc-master -name tst\* -o -name test\* | grep printf
glibc-master/stdio-common/tst-swprintf.c
glibc-master/stdio-common/test-vfprintf.c
glibc-master/stdio-common/tst-printf-round.c
glibc-master/stdio-common/tst-printf.c
glibc-master/stdio-common/tst-sprintf3.c
glibc-master/stdio-common/tst-printf.sh
glibc-master/stdio-common/tst-sprintf2.c
glibc-master/stdio-common/tst-sprintf.c
glibc-master/stdio-common/tst-obprintf.c
glibc-master/stdio-common/tst-wc-printf.c
glibc-master/stdio-common/tst-printfsz.c

Could you look into doing the same for printf?

I probably should...

[kcc]

asan/lit_tests/TestCases/printf-2.c

2

please make these tests run in 3 modes:

ASAN_OPTIONS=check_printf=0
ASAN_OPTIONS=check_printf=1
ASAN_OPTIONS=""

we need to test that the flag affects the behavior and that it's default value is what we expect it to be.

(use FileCheck --check-prefix=CHECK-ON and --check-prefix=CHECK-OFF)

sanitizer_common/sanitizer_flags.h

60

I actually changed my mind.
Please make if false by default for now.
After this patch is committed we'll test it a bit more before enabling by default.

[ygribov]

asan/lit_tests/TestCases/printf-2.c

2

Ok.

[ygribov]

asan/lit_tests/TestCases/printf-2.c

2

BTW how do you suggest the check_printf=0 case? In my understanding we'll have UB with absolutely unpredictable behavior...

[ygribov]

asan/lit_tests/TestCases/printf-2.c

2

That's not that bad.
If you allocate a heap block using asan's allocator,
the behavior of an uncheck-ed out-of-bounds read is quite predictable

I tend to agree but e.g. GCC team tends to reject tests that trigger UB. Also things like this tend to be rather unstable with respect to glibc/kernel versions.

I'll do this if you're adamant though.

[ygribov]

New revision based on comments from glider, eugenis and kcc.

Main changes:

• fixed sanitizer_scanf_interceptor_test.cc
• update printf path based on Open Group std
• fixed several bugs based on glibc printf tests (manifested in segvs and false alarms)
• check_printf is now false by default

[eugenis]

lib/sanitizer_common/sanitizer_common_interceptors_scanf.inc

90 ↗

(On Diff #6394)

According to the manual, field width can be "*", too. The number-or-star parsing logic could be factored out in a separate function.

98 ↗

(On Diff #6394)

This does not look right. %m in printf is a complete conversion specifier, it should end parsing of the current directive. And it seems like "m" could go into convSpecifier, and printErrno could be removed.

168 ↗

(On Diff #6394)

At this point allowGnuMalloc is true. It does not make sense for output to be true. Please remove this and instead check this invariant at the beginning of the functions (!(allowGnuMalloc && output)).

293 ↗

(On Diff #6394)

Are these guys ([...]) even allowed in printf?

360 ↗

(On Diff #6394)

Does it really need to be a macro?

[ygribov]

lib/sanitizer_common/sanitizer_common_interceptors_scanf.inc

90 ↗

(On Diff #6394)

According to the manual, field width can be "*", too

Yup, this is handled by dir->suppressed above)

The number-or-star parsing logic could be factored out in a separate function.

Agreed.

98 ↗

(On Diff #6394)

%m in printf is a complete conversion specifier, it should end parsing of the current directive

I agree that we are currently parsing a wider format language than really necessary. But same is true for %n in original scanf implementation and we didn't bother handling it.

360 ↗

(On Diff #6394)

I was under impression that pointers to va_list are not portable C but http://gcc.gnu.org/ml/gcc-help/1999-q3n/msg00283.html suggests that they're sane. I'll give it a try tomorrow.

[ygribov]

lib/sanitizer_common/sanitizer_common_interceptors_scanf.inc

168 ↗

(On Diff #6394)

Agreed.

293 ↗

(On Diff #6394)

No, they aren't. But why do you ask the question here?

[ygribov]

lib/sanitizer_common/sanitizer_common_interceptors_scanf.inc

98 ↗

(On Diff #6394)

And it seems like "m" could go into convSpecifier, and printErrno could be removed.

Agreed to this one, will fix.

360 ↗

(On Diff #6394)

Ok, even though standard kind of allows this, GCC has a long and famous history of pointers to va_list not working (see e.g. http://gcc.gnu.org/bugzilla/show_bug.cgi?id=14557). Looks like passing va_list by reference is unportable.

[eugenis]

lib/sanitizer_common/sanitizer_common_interceptors_scanf.inc

90 ↗

(On Diff #6394)

Oh, right. This is a bit confusing, could you add a separate field to FormatDirective instead of reusing "suppressed" here?

98 ↗

(On Diff #6394)

What's wrong with %n? We parse it as convSpecifier which can not have anything going after it.
It seems like simply changing the code here to

if (*p == 'm' && !output) {

dir->allocate=true;
++p;

}

would do the trick.

293 ↗

(On Diff #6394)

Then it would make sense to handle this case earlier, in format_parse_next. Probably not a big deal.

[ygribov]

lib/sanitizer_common/sanitizer_common_interceptors_scanf.inc

90 ↗

(On Diff #6394)

Already done. I've also decided to split format_parse_next to {printf,scanf}_parse_next because they are different enough.

98 ↗

(On Diff #6394)

Ok, should be fixed by splitting format_parse_next mentioned above.

[ygribov]

Major changes:

• split format_parse_next to 2 functions
• same for scanf_get_value_size
• safer check of buffer size in sprintf

Issues found by eugenis should be fixed now.

[eugenis]

lib/sanitizer_common/sanitizer_common_interceptors_scanf.inc

204 ↗

(On Diff #6410)

It never returns null pointer.

212 ↗

(On Diff #6410)

maybe_parse_signed ?

420 ↗

(On Diff #6410)

Code duplication between scanf and printf parsers. Please factor this out.

[ygribov]

lib/sanitizer_common/sanitizer_common_interceptors_scanf.inc

204 ↗

(On Diff #6410)

True, just trying to be safe here.

212 ↗

(On Diff #6410)

My bad.

420 ↗

(On Diff #6410)

Will do.

[ygribov]

Hey guys, I've fallen ill so won't be able to respond quickly.

Meanwhile, are there any other problems that you want fixed?

[eugenis]

I'll do another review pass tomorrow morning.
Get well!

[eugenis]

lib/sanitizer_common/sanitizer_common_interceptors.inc

697 ↗

(On Diff #6410)

Unless I'm missing something, you could use REAL(strlen)(str) here instead of calling *printf twice.

lib/sanitizer_common/sanitizer_common_interceptors_scanf.inc

161 ↗

(On Diff #6410)

Should it be lengthModifier[0]?
Looks like a bug in the original code.

[ygribov]

lib/sanitizer_common/sanitizer_common_interceptors.inc

697 ↗

(On Diff #6410)

Hm but str isn't necessarily a string - it's just plain array of chars. I don't think we can call strlen on it.

lib/sanitizer_common/sanitizer_common_interceptors_scanf.inc

161 ↗

(On Diff #6410)

1. Bug or some weird GNU scanf (mis)feature. Who is the author of this piece?
2. How do you guys normally treat situations when both change code and move it to a different place for clarity? Do you make two separate commits to make reviewers life easier?

[eugenis]

lib/sanitizer_common/sanitizer_common_interceptors.inc

697 ↗

(On Diff #6410)

I think *printf always end the output with a null character, and never print null in the middle, but I don't see a clear indication of that in the spec.

lib/sanitizer_common/sanitizer_common_interceptors_scanf.inc

161 ↗

(On Diff #6410)

Me :)
I think it's a bug, it's meant to check that %p has no length modifier.
Two commits are good, but people often don't bother.
In this case it's ok to make the change in the same commit, or whatever is more convenient for you.

[ygribov]

lib/sanitizer_common/sanitizer_common_interceptors.inc

697 ↗

(On Diff #6410)

Sorry, I must be missing something... My intent with REAL(vname2) (which is actually REAL(vsnprintf)) is to estimate number of chars that will be written. 0 tells vsnprintf that it should not _write_ anything, only estimate number of chars. We could simply pass NULL instead of str and it wouldn't change anything. So why do you bother about str?

lib/sanitizer_common/sanitizer_common_interceptors_scanf.inc

161 ↗

(On Diff #6410)

I'll update the code and see if tests seize to work then.

[eugenis]

lib/sanitizer_common/sanitizer_common_interceptors.inc

697 ↗

(On Diff #6410)

I'm just worried that it makes *printf quite a bit heavier - we now parse the format string 3 times (2 in libc *printf and 1 in our code). Alternatively, we could do 1 + 1, and then figure out the length of the resulting string with strlen() after the REAL *printf.

[ygribov]

lib/sanitizer_common/sanitizer_common_interceptors.inc

697 ↗

(On Diff #6410)

Agreed but then we may get stack overflow before Asan is able to react. Looks like a trade-off between speed and security. Does libsanitizer have general preference for any of those?

IMHO sprintf is already so slow that we may not need to bother so much.

[ygribov]

And sprintf usage is discouraged by all coding standards and static analyzers anyway :)

[eugenis]

lib/sanitizer_common/sanitizer_common_interceptors.inc

697 ↗

(On Diff #6410)

Now I must be missing something. What would be the cause of stack overflow?

That would slow down sprintf (and snprintf, etc) by a factor of 1.5 (roughly speaking). That may be a big hit exactly because sprintf is so slow. I'm assuming that calculation the length of the output by passing 0 as size takes about the same time as actually formatting the output.

[ygribov]

lib/sanitizer_common/sanitizer_common_interceptors.inc

697 ↗

(On Diff #6410)

Now I must be missing something. What would be the cause of stack overflow?

A call to REAL(sprintf) with unchecked target buffer size.

That would slow down sprintf (and snprintf, etc) by a factor of 1.5 (roughly speaking).

Yup, as I said I agree that this adds a penalty. Let's see if we can now come to conclusion on buffer overflow part and see which one is more important.

[eugenis]

lib/sanitizer_common/sanitizer_common_interceptors.inc

697 ↗

(On Diff #6410)

I see. But that's the same call that the application does, i.e. the overflow would have happened without sanitizer, too. And we will most likely catch it, but after sprintf corrupts some memory.

I think its common to use the strlen way. Interceptors like strcat, strcpy call the real function without checking that the string is actually null-terminated, and examine the result afterwards.

[ygribov]

lib/sanitizer_common/sanitizer_common_interceptors.inc

697 ↗

(On Diff #6410)

we will most likely catch it, but after sprintf corrupts some memory.

After some internal fighting I have to agree with that. The claim would be wrong on platforms with ascending stacks but I'm unaware of any living instance (last I heard Aarch64 failed at that).

I think its common to use the strlen way.
Interceptors like strcat, strcpy call the real function
without checking

I see, so we favor perf indeed. I'll remove the first sprintf call then.

BTW do we mention somewhere on our wiki that our interceptors are not 100% secure?

[ygribov]

New diff based on Evgeny's comments.

[eugenis]

Could you rebuild the new patch from projects/compiler-rt directory
instead of projects/compiler-rt/lib? Right now the incremental diff
between two last revisions is completely broken.

[ygribov]

Rebased diff to compiler-rt root.

[ygribov]

Frankly I'd expect Phabricator to catch silly errors like this...

[eugenis]

LGTM w/ nit
But please wait for samsonov's review.

lib/sanitizer_common/sanitizer_common_interceptors.inc

691 ↗

(On Diff #6502)

vname2 can be removed

697 ↗

(On Diff #6410)

I don't think we claim anywhere that ASan is 100% reliable, or that it catches all the bugs ;)

[samsonov]

lib/asan/lit_tests/TestCases/printf-1.c

4 ↗

(On Diff #6502)

Just use "%t 2>&1" to test it with default ASAN_OPTIONS.

6 ↗

(On Diff #6502)

I'd suggest to merge all these test cases into one, as we're really testing one thing here. That would save some lines of code and would make modifying and extending the test case easier, and the tests will run faster (you will compile/link a single binary instead of 5 binaries). You can control the printf(...) call you're testing with an argv. See TestCases/stack-buffer-overflow-with-position.cc

lib/asan/lit_tests/TestCases/printf-2.c

20 ↗

(On Diff #6502)

Could you match a file/line in the stack trace in error report? E.g. see TestCases/null_deref.cc

lib/sanitizer_common/sanitizer_common_interceptors.inc

573 ↗

(On Diff #6502)

Just "#include", we don't seem to use "# " indentation in another places in this file.

lib/sanitizer_common/sanitizer_common_interceptors_scanf.inc

25 ↗

(On Diff #6502)

Quick check: is it OK you will consume the first "-" in "--" string?

30 ↗

(On Diff #6502)

Please clarify in function name or comment why "0" or "-0" are unexpected.

61 ↗

(On Diff #6502)

remove empty line

233 ↗

(On Diff #6502)

Is it ok the code originally parsed only unsigned integers as the field width, but now it will treat "-9" as a field width "9"?

348 ↗

(On Diff #6502)

I don't like this diagnostics: we don't even print what this unknown specifier is. We should either do this, or silently exit.

508 ↗

(On Diff #6502)

same here.

lib/sanitizer_common/tests/sanitizer_scanf_interceptor_test.cc

61 ↗

(On Diff #6502)

This one is also const.

[ygribov]

lib/asan/lit_tests/TestCases/printf-1.c

4 ↗

(On Diff #6502)

Ok.

6 ↗

(On Diff #6502)

I don't quite agree. Different tests trigger different types of memory errors so CHECKs will be different. Also printed messages in some tests differ.

lib/asan/lit_tests/TestCases/printf-2.c

20 ↗

(On Diff #6502)

Well, ok although I'd consider this an overkill.

lib/sanitizer_common/sanitizer_common_interceptors.inc

573 ↗

(On Diff #6502)

+1

691 ↗

(On Diff #6502)

+1

lib/sanitizer_common/sanitizer_common_interceptors_scanf.inc

25 ↗

(On Diff #6502)

%-- is invalid syntax anyway so I think it's ok.

30 ↗

(On Diff #6502)

This is Evegeny's code so I have no idea.

61 ↗

(On Diff #6502)

Ok.

233 ↗

(On Diff #6502)

Well, the spec does not mention that width in scanf must be positive but GNU scanf rejects negatives. I'll replace with parse_number.

348 ↗

(On Diff #6502)

This warning is mainly to inform us that we failed to parse some format spec and I think it's rather important for debugging purposes. What if I replace it with VReport(1, ...) ?

lib/sanitizer_common/tests/sanitizer_scanf_interceptor_test.cc

61 ↗

(On Diff #6502)

+1

[eugenis]

lib/sanitizer_common/sanitizer_common_interceptors_scanf.inc

30 ↗

(On Diff #6502)

Could it be different for printf vs scanf?

348 ↗

(On Diff #6502)

The warning is good, but to be useful it should print a bit more info. Otherwise in a big program it is not at all clear what to do about it.

[samsonov]

lib/asan/lit_tests/TestCases/printf-1.c

6 ↗

(On Diff #6502)

Well, ok, leaving this to you. If you think that merging the cases would complicate things too much, leave it as is.

lib/asan/lit_tests/TestCases/printf-2.c

20 ↗

(On Diff #6502)

We generally don't want many internal frames from ASan runtime to appear in the error report. So it's nice to verify that error report looks somewhat like this:
#0 __interceptor_printf ....
#1 main printf-2.c:16

lib/sanitizer_common/sanitizer_common_interceptors_scanf.inc

25 ↗

(On Diff #6502)

Alright, I agree that %-- is just invalid scanf/printf syntax and we may not necessarily parse it correctly, but now we move this to function "maybe_parse_signed", and its name doesn't tell much about this hidden assumptions ("-" consumed from "--", "0" results in NULL returned), so one could use it in a wrong way somewhere else. IMO It's better to specify this assumptions explicitly here.

30 ↗

(On Diff #6502)

See above

[ygribov]

lib/sanitizer_common/sanitizer_common_interceptors_scanf.inc

30 ↗

(On Diff #6502)

Closer study of scanf/printf spec reveals that width/precision are unsigned both for printf and scanf. But printf may have '-' or '+' flag in front of width.

Anyway I don't know why you check for <= 0 here.

348 ↗

(On Diff #6502)

The best solution would be to remember pointers to begin and end of specifier in format string. We can then use them to print specifier here.

[ygribov]

lib/asan/lit_tests/TestCases/printf-2.c

20 ↗

(On Diff #6502)

Got it.

lib/sanitizer_common/sanitizer_common_interceptors_scanf.inc

25 ↗

(On Diff #6502)

I agree that whole parse_number/parse_signed thing is a bit messy. Let me see if I can prettify it with next commit.

[ygribov]

lib/sanitizer_common/sanitizer_common_interceptors_scanf.inc

30 ↗

(On Diff #6502)

Ok, I got it: scanf is only allowed to have a non-zero width so this check needs to be moved to scanf_parse_next. All this syntax peculiarities are going to drive me crazy.

[ygribov]

Updates based on feedback from Evgeny&Alex.

What's missing:

• I haven't added the @LINE directives; currently ASan reports #0 0x431593 in printf_common #1 0x431a94 in __interceptor_vprintf #2 0x42b2fd in printf #3 0x47a897 in main

which probably is not what we want but I don't think that we should fix it in this patch.

Regarding glibc tests: I've ran them and verified that ASan does not cause segfaults or test failures (it does print couple of warnings on invalid format strings). Is this what you guys wanted?

[ygribov]

Alex, Evgeny,

Do you guys want anything else to go into this revision?

-Y

[samsonov]

I will look at it now. Sorry for delay.

[glider]

A couple of nits.

lib/sanitizer_common/tests/sanitizer_scanf_interceptor_test.cc

37 ↗

(On Diff #6503)

Can you please add a comment above describing why NOLINT is necessary here?

43 ↗

(On Diff #6503)

At least two spaces between the code and the comment, please (you'll need to reformat the rest of declarations)

[samsonov]

Only a few minor nits (I've also committed part of this patch already). I think it's fine for submission (once you're re-upload the diff addressing the comments).

lib/asan/lit_tests/TestCases/printf-5.c

4 ↗

(On Diff #6503)

Once again, what's the point of testing the default configuration? In this way you'll have to update the tests on a flag flip, is this really what we want? Can we remove it, or at least have a single test for default configuration?

lib/sanitizer_common/sanitizer_common_interceptors_scanf.inc

48 ↗

(On Diff #6503)

q is always non-NULL

451 ↗

(On Diff #6503)

Please either make it a function (I see, va_list might be a problem), or name it as SKIP_SCALAR_ARG

lib/sanitizer_common/sanitizer_printf.cc

98 ↗

(On Diff #6503)

I've submitted this part in r199724 (it's not directly related to printf interceptors).

[kcc]

lib/asan/lit_tests/TestCases/printf-5.c

4 ↗

(On Diff #6503)

This was my suggestion, I think this *is* what we want.

[ygribov]

lib/asan/lit_tests/TestCases/printf-5.c

4 ↗

(On Diff #6503)

Yeah, I guess Kostya wants to explicitly test that printf checks are disabled by default for now.

lib/sanitizer_common/sanitizer_common_interceptors_scanf.inc

48 ↗

(On Diff #6503)

+1

451 ↗

(On Diff #6503)

Will do.

lib/sanitizer_common/tests/sanitizer_scanf_interceptor_test.cc

37 ↗

(On Diff #6503)

I copy-pasted this from TEST(SanitizerCommonInterceptors, Scanf). Seems to work with NOLINT removed so I'll update the patch.

43 ↗

(On Diff #6503)

Likewise.

[ygribov]

Minor updates based on review.

[samsonov]

LGTM.

Submitted as r199729. Huge thanks for working on this!

[ygribov]

Np, waiting for real-world feedback from you.

asan/lit_tests/TestCases/printf-1.c

+// RUN: %clang_asan -O2 %s -o %t
+// RUN: %t 2>&1 | FileCheck %s
+
+#include <stdio.h>
+int main() {
+  volatile char c = '0';
+  volatile int x = 12;
+  volatile char s[] = "34";
+  printf("%c %d %s\n", c, x, s);
+  return 0;
+  // Check that printf works fine under Asan.
+  // CHECK: 0 12 34
+}

asan/lit_tests/TestCases/printf-2.c

+// RUN: %clang_asan -O2 %s -o %t
+// RUN: not %t 2>&1 | FileCheck %s
+
+#include <stdio.h>
+#include <string.h>
+#include <stdlib.h>
+int main() {
+  volatile char c = '0';
+  volatile int x = 12;
+  volatile char s[] = "34";
+  char *p = strdup((const char *)s);
+  free(p);
+  printf("%c %d %s\n", c, x, p);
+  return 0;
+  // Check that %s is checked
+  // CHECK: heap-use-after-free
+  // CHECK-NOT: 0 12 34
+}

asan/lit_tests/TestCases/printf-3.c

+// RUN: %clang_asan -O2 %s -o %t
+// RUN: not %t 2>&1 | FileCheck %s
+
+#include <stdio.h>
+#include <string.h>
+#include <stdlib.h>
+int main() {
+  volatile char c = '0';
+  volatile int x = 12;
+  volatile char s[] = "34";
+  volatile int n[1];
+  printf("%c %d %s%n\n", c, x, s, &n[1]);
+  return 0;
+  // Check that %n is checked
+  // CHECK: stack-buffer-overflow
+  // CHECK-NOT: 0 12 34
+}

asan/lit_tests/TestCases/printf-4.c

+// RUN: %clang_asan -O2 %s -o %t
+// RUN: not %t 2>&1 | FileCheck %s
+
+#include <stdio.h>
+#include <string.h>
+#include <stdlib.h>
+int main() {
+  volatile char c = '0';
+  volatile int x = 12;
+  volatile char s[] = "34";
+  volatile char buf[2];
+  sprintf((char *)buf, "%c %d %s\n", c, x, s);
+  return 0;
+  // Check that size of output buffer is checked
+  // CHECK: stack-buffer-overflow
+  // CHECK-NOT: 0 12 34
+}

asan/lit_tests/TestCases/printf-5.c

+// RUN: %clang_asan -O2 %s -o %t
+// RUN: not %t 2>&1 | FileCheck %s
+
+#include <stdio.h>
+#include <string.h>
+#include <stdlib.h>
+int main() {
+  volatile char c = '0';
+  volatile int x = 12;
+  volatile char s[] = "34";
+  volatile char fmt[2];
+  memcpy((char *)fmt, "%c %d %s\n", sizeof(fmt));
+  printf(fmt, c, x, s);
+  return 0;
+  // Check that format string is checked
+  // CHECK: stack-buffer-overflow
+  // CHECK-NOT: 0 12 34
+}

msan/msan_interceptors.cc

 #define COMMON_INTERCEPTOR_SET_THREAD_NAME(ctx, name) \
   do {                                                \
   } while (false)  // FIXME
 #define COMMON_INTERCEPTOR_SET_PTHREAD_NAME(ctx, thread, name) \
   do {                                                         \
   } while (false)  // FIXME
 #define COMMON_INTERCEPTOR_BLOCK_REAL(name) REAL(name)
 #define COMMON_INTERCEPTOR_ON_EXIT(ctx) OnExit()
+// FIXME: update Msan to use common printf interceptors
+#define SANITIZER_INTERCEPT_PRINTF 0
 #include "sanitizer_common/sanitizer_common_interceptors.inc"
 
 #define COMMON_SYSCALL_PRE_READ_RANGE(p, s) CHECK_UNPOISONED(p, s)
 #define COMMON_SYSCALL_PRE_WRITE_RANGE(p, s) \
   do {                                       \
   } while (false)
 #define COMMON_SYSCALL_POST_READ_RANGE(p, s) \
   do {                                       \

sanitizer_common/sanitizer_common_interceptors.inc

 //   COMMON_INTERCEPTOR_MUTEX_UNLOCK
 //   COMMON_INTERCEPTOR_MUTEX_REPAIR
 //   COMMON_INTERCEPTOR_SET_PTHREAD_NAME
 //   COMMON_INTERCEPTOR_HANDLE_RECVMSG
 //===----------------------------------------------------------------------===//
 #include "interception/interception.h"
 #include "sanitizer_platform_interceptors.h"
 
 #include <stdarg.h>
 
 #if SANITIZER_WINDOWS && !defined(va_copy)
 #define va_copy(dst, src) ((dst) = (src))
 #endif // _WIN32
 
 #ifndef COMMON_INTERCEPTOR_INITIALIZE_RANGE
 #define COMMON_INTERCEPTOR_INITIALIZE_RANGE(ctx, p, size) {}
 #endif
INTERCEPTOR(char *, strptime, char *s, char *format, __sanitizer_tm *tm) {
   }
   return res;
 }
 #define INIT_STRPTIME COMMON_INTERCEPT_FUNCTION(strptime);
 #else
 #define INIT_STRPTIME
 #endif
 
-#if SANITIZER_INTERCEPT_SCANF
-
-#include "sanitizer_common_interceptors_scanf.inc"
+#if SANITIZER_INTERCEPT_SCANF || SANITIZER_INTERCEPT_PRINTF
+# include "sanitizer_common_interceptors_scanf.inc"
 
+#define FORMAT_INTERCEPTOR_IMPL(name, vname, ...)                              \
+  {                                                                            \
+    void *ctx;                                                                 \
+    va_list ap;                                                                \
+    va_start(ap, format);                                                      \
+    COMMON_INTERCEPTOR_ENTER(ctx, vname, __VA_ARGS__, ap);                     \
+    int res = vname(__VA_ARGS__, ap);                                          \
+    va_end(ap);                                                                \
+    return res;                                                                \
+  }
+
+#endif
+
+#if SANITIZER_INTERCEPT_SCANF
+
 #define VSCANF_INTERCEPTOR_IMPL(vname, allowGnuMalloc, ...)                    \
   {                                                                            \
     void *ctx;                                                                 \
     COMMON_INTERCEPTOR_ENTER(ctx, vname, __VA_ARGS__);                         \
     va_list aq;                                                                \
     va_copy(aq, ap);                                                           \
     int res = REAL(vname)(__VA_ARGS__);                                        \
     if (res > 0)                                                               \

 INTERCEPTOR(int, __isoc99_vsscanf, const char *str, const char *format,
             va_list ap)
 VSCANF_INTERCEPTOR_IMPL(__isoc99_vsscanf, false, str, format, ap)
 
 INTERCEPTOR(int, __isoc99_vfscanf, void *stream, const char *format, va_list ap)
 VSCANF_INTERCEPTOR_IMPL(__isoc99_vfscanf, false, stream, format, ap)
 #endif  // SANITIZER_INTERCEPT_ISOC99_SCANF
 
-#define SCANF_INTERCEPTOR_IMPL(name, vname, ...)                               \
-  {                                                                            \
-    void *ctx;                                                                 \
-    va_list ap;                                                                \
-    va_start(ap, format);                                                      \
-    COMMON_INTERCEPTOR_ENTER(ctx, vname, __VA_ARGS__, ap);                     \
-    int res = vname(__VA_ARGS__, ap);                                          \
-    va_end(ap);                                                                \
-    return res;                                                                \
-  }
-
 INTERCEPTOR(int, scanf, const char *format, ...)
-SCANF_INTERCEPTOR_IMPL(scanf, vscanf, format)
+FORMAT_INTERCEPTOR_IMPL(scanf, vscanf, format)
 
 INTERCEPTOR(int, fscanf, void *stream, const char *format, ...)
-SCANF_INTERCEPTOR_IMPL(fscanf, vfscanf, stream, format)
+FORMAT_INTERCEPTOR_IMPL(fscanf, vfscanf, stream, format)
 
 INTERCEPTOR(int, sscanf, const char *str, const char *format, ...)
-SCANF_INTERCEPTOR_IMPL(sscanf, vsscanf, str, format)
+FORMAT_INTERCEPTOR_IMPL(sscanf, vsscanf, str, format)
 
 #if SANITIZER_INTERCEPT_ISOC99_SCANF
 INTERCEPTOR(int, __isoc99_scanf, const char *format, ...)
-SCANF_INTERCEPTOR_IMPL(__isoc99_scanf, __isoc99_vscanf, format)
+FORMAT_INTERCEPTOR_IMPL(__isoc99_scanf, __isoc99_vscanf, format)
 
 INTERCEPTOR(int, __isoc99_fscanf, void *stream, const char *format, ...)
-SCANF_INTERCEPTOR_IMPL(__isoc99_fscanf, __isoc99_vfscanf, stream, format)
+FORMAT_INTERCEPTOR_IMPL(__isoc99_fscanf, __isoc99_vfscanf, stream, format)
 
 INTERCEPTOR(int, __isoc99_sscanf, const char *str, const char *format, ...)
-SCANF_INTERCEPTOR_IMPL(__isoc99_sscanf, __isoc99_vsscanf, str, format)
+FORMAT_INTERCEPTOR_IMPL(__isoc99_sscanf, __isoc99_vsscanf, str, format)
 #endif
 
 #endif
 
 #if SANITIZER_INTERCEPT_SCANF
 #define INIT_SCANF                    \
   COMMON_INTERCEPT_FUNCTION(scanf);   \
   COMMON_INTERCEPT_FUNCTION(sscanf);  \
#define INIT_ISOC99_SCANF                      \
   COMMON_INTERCEPT_FUNCTION(__isoc99_fscanf);  \
   COMMON_INTERCEPT_FUNCTION(__isoc99_vscanf);  \
   COMMON_INTERCEPT_FUNCTION(__isoc99_vsscanf); \
   COMMON_INTERCEPT_FUNCTION(__isoc99_vfscanf);
 #else
 #define INIT_ISOC99_SCANF
 #endif
 
+#if SANITIZER_INTERCEPT_PRINTF
+
+#define VPRINTF_INTERCEPTOR_ENTER(vname, ...)                                  \
+  void *ctx;                                                                   \
+  COMMON_INTERCEPTOR_ENTER(ctx, vname, __VA_ARGS__);                           \
+  va_list aq;                                                                  \
+  va_copy(aq, ap);
+
+#define VPRINTF_INTERCEPTOR_RETURN()                                           \
+  va_end(aq);
+
+#define VPRINTF_INTERCEPTOR_IMPL(vname, ...)                                   \
+  {                                                                            \
+    VPRINTF_INTERCEPTOR_ENTER(vname, __VA_ARGS__);                             \
+    if (common_flags()->check_printf)                                          \
+      printf_common(ctx, format, aq);                                          \
+    int res = REAL(vname)(__VA_ARGS__);                                        \
+    VPRINTF_INTERCEPTOR_RETURN();                                              \
+    return res;                                                                \
+  }
+
+#define VSPRINTF_INTERCEPTOR_IMPL(vname, str, ...)                             \
+  {                                                                            \
+    VPRINTF_INTERCEPTOR_ENTER(vname, str, __VA_ARGS__)                         \
+    if (common_flags()->check_printf) {                                        \
+      printf_common(ctx, format, aq);                                          \
+    }                                                                          \
+    int res = REAL(vname)(str, __VA_ARGS__);                                   \
+    if (res > 0 && common_flags()->check_printf) {                             \
+      COMMON_INTERCEPTOR_WRITE_RANGE(ctx, str, res + 1);                       \
+    }                                                                          \
+    VPRINTF_INTERCEPTOR_RETURN();                                              \
+    return res;                                                                \
+  }
+
+#define VSNPRINTF_INTERCEPTOR_IMPL(vname, str, size, ...)                      \
+  {                                                                            \
+    VPRINTF_INTERCEPTOR_ENTER(vname, str, size, __VA_ARGS__)                   \
+    if (common_flags()->check_printf) {                                        \
+      COMMON_INTERCEPTOR_WRITE_RANGE(ctx, str, size);                          \
+      printf_common(ctx, format, aq);                                          \
+    }                                                                          \
+    int res = REAL(vname)(str, size, __VA_ARGS__);                             \
+    if (res > 0 && common_flags()->check_printf) {                             \
+      COMMON_INTERCEPTOR_WRITE_RANGE(ctx, str, res + 1);                       \
+    }                                                                          \
+    VPRINTF_INTERCEPTOR_RETURN();                                              \
+    return res;                                                                \
+  }
+
+#define VASPRINTF_INTERCEPTOR_IMPL(vname, strp, ...)                           \
+  {                                                                            \
+    VPRINTF_INTERCEPTOR_ENTER(vname, strp, __VA_ARGS__)                        \
+    if (common_flags()->check_printf) {                                        \
+      COMMON_INTERCEPTOR_WRITE_RANGE(ctx, strp, sizeof(char *));               \
+      printf_common(ctx, format, aq);                                          \
+    }                                                                          \
+    int res = REAL(vname)(strp, __VA_ARGS__);                                  \
+    if (res > 0 && common_flags()->check_printf) {                             \
+      COMMON_INTERCEPTOR_WRITE_RANGE(ctx, *strp, res);                         \
+    }                                                                          \
+    VPRINTF_INTERCEPTOR_RETURN();                                              \
+    return res;                                                                \
+  }
+
+INTERCEPTOR(int, vprintf, const char *format, va_list ap)
+VPRINTF_INTERCEPTOR_IMPL(vprintf, format, ap)
+
+INTERCEPTOR(int, vfprintf, void *stream, const char *format, va_list ap)
+VPRINTF_INTERCEPTOR_IMPL(vfprintf, stream, format, ap)
+
+INTERCEPTOR(int, vsprintf, char *str, const char *format, va_list ap)
+VSPRINTF_INTERCEPTOR_IMPL(vsprintf, str, format, ap)
+
+INTERCEPTOR(int, vsnprintf, char *str, SIZE_T size, const char *format,
+            va_list ap)
+VSNPRINTF_INTERCEPTOR_IMPL(vsnprintf, str, size, format, ap)
+
+INTERCEPTOR(int, vasprintf, char **strp, const char *format, va_list ap)
+VASPRINTF_INTERCEPTOR_IMPL(vasprintf, strp, format, ap)
+
+#if SANITIZER_INTERCEPT_ISOC99_PRINTF
+INTERCEPTOR(int, __isoc99_vprintf, const char *format, va_list ap)
+VPRINTF_INTERCEPTOR_IMPL(__isoc99_vprintf, format, ap)
+
+INTERCEPTOR(int, __isoc99_vfprintf, void *stream, const char *format,
+            va_list ap)
+VPRINTF_INTERCEPTOR_IMPL(__isoc99_vfprintf, stream, format, ap)
+
+INTERCEPTOR(int, __isoc99_vsprintf, char *str, const char *format,
+            va_list ap)
+VSPRINTF_INTERCEPTOR_IMPL(__isoc99_vsprintf, str, format, ap)
+
+INTERCEPTOR(int, __isoc99_vsnprintf, char *str, SIZE_T size, const char *format,
+            va_list ap)
+VSNPRINTF_INTERCEPTOR_IMPL(__isoc99_vsnprintf, str, size, format, ap)
+
+#endif  // SANITIZER_INTERCEPT_ISOC99_PRINTF
+
+INTERCEPTOR(int, printf, const char *format, ...)
+FORMAT_INTERCEPTOR_IMPL(printf, vprintf, format)
+
+INTERCEPTOR(int, fprintf, void *stream, const char *format, ...)
+FORMAT_INTERCEPTOR_IMPL(fprintf, vfprintf, stream, format)
+
+INTERCEPTOR(int, sprintf, char *str, const char *format, ...) // NOLINT
+FORMAT_INTERCEPTOR_IMPL(sprintf, vsprintf, str, format) // NOLINT
+
+INTERCEPTOR(int, snprintf, char *str, SIZE_T size, const char *format, ...)
+FORMAT_INTERCEPTOR_IMPL(snprintf, vsnprintf, str, size, format)
+
+INTERCEPTOR(int, asprintf, char **strp, const char *format, ...)
+FORMAT_INTERCEPTOR_IMPL(asprintf, vasprintf, strp, format)
+
+#if SANITIZER_INTERCEPT_ISOC99_PRINTF
+INTERCEPTOR(int, __isoc99_printf, const char *format, ...)
+FORMAT_INTERCEPTOR_IMPL(__isoc99_printf, __isoc99_vprintf, format)
+
+INTERCEPTOR(int, __isoc99_fprintf, void *stream, const char *format, ...)
+FORMAT_INTERCEPTOR_IMPL(__isoc99_fprintf, __isoc99_vfprintf, stream, format)
+
+INTERCEPTOR(int, __isoc99_sprintf, char *str, const char *format, ...)
+FORMAT_INTERCEPTOR_IMPL(__isoc99_sprintf, __isoc99_vsprintf, str, format)
+
+INTERCEPTOR(int, __isoc99_snprintf, char *str, SIZE_T size,
+            const char *format, ...)
+FORMAT_INTERCEPTOR_IMPL(__isoc99_snprintf, __isoc99_vsnprintf, str, size,
+                        format)
+
+#endif  // SANITIZER_INTERCEPT_ISOC99_PRINTF
+
+#endif  // SANITIZER_INTERCEPT_PRINTF
+
+#if SANITIZER_INTERCEPT_PRINTF
+#define INIT_PRINTF                     \
+  COMMON_INTERCEPT_FUNCTION(printf);    \
+  COMMON_INTERCEPT_FUNCTION(sprintf);   \
+  COMMON_INTERCEPT_FUNCTION(snprintf);  \
+  COMMON_INTERCEPT_FUNCTION(asprintf);  \
+  COMMON_INTERCEPT_FUNCTION(fprintf);   \
+  COMMON_INTERCEPT_FUNCTION(vprintf);   \
+  COMMON_INTERCEPT_FUNCTION(vsprintf);  \
+  COMMON_INTERCEPT_FUNCTION(vsnprintf); \
+  COMMON_INTERCEPT_FUNCTION(vasprintf); \
+  COMMON_INTERCEPT_FUNCTION(vfprintf);
+#else
+#define INIT_PRINTF
+#endif
+
+#if SANITIZER_INTERCEPT_ISOC99_PRINTF
+#define INIT_ISOC99_PRINTF                       \
+  COMMON_INTERCEPT_FUNCTION(__isoc99_printf);    \
+  COMMON_INTERCEPT_FUNCTION(__isoc99_sprintf);   \
+  COMMON_INTERCEPT_FUNCTION(__isoc99_snprintf);  \
+  COMMON_INTERCEPT_FUNCTION(__isoc99_fprintf);   \
+  COMMON_INTERCEPT_FUNCTION(__isoc99_vprintf);   \
+  COMMON_INTERCEPT_FUNCTION(__isoc99_vsprintf);  \
+  COMMON_INTERCEPT_FUNCTION(__isoc99_vsnprintf); \
+  COMMON_INTERCEPT_FUNCTION(__isoc99_vfprintf);
+#else
+#define INIT_ISOC99_PRINTF
+#endif
+
 #if SANITIZER_INTERCEPT_IOCTL
 #include "sanitizer_common_interceptors_ioctl.inc"
 INTERCEPTOR(int, ioctl, int d, unsigned request, void *arg) {
   void *ctx;
   COMMON_INTERCEPTOR_ENTER(ctx, ioctl, d, request, arg);
 
   CHECK(ioctl_initialized);
 
#define SANITIZER_COMMON_INTERCEPTORS_INIT \
   INIT_WRITEV;                             \
   INIT_PWRITEV;                            \
   INIT_PWRITEV64;                          \
   INIT_PRCTL;                              \
   INIT_LOCALTIME_AND_FRIENDS;              \
   INIT_STRPTIME;                           \
   INIT_SCANF;                              \
   INIT_ISOC99_SCANF;                       \
+  INIT_PRINTF;                             \
+  INIT_ISOC99_PRINTF;                      \
   INIT_FREXP;                              \
   INIT_FREXPF_FREXPL;                      \
   INIT_GETPWNAM_AND_FRIENDS;               \
   INIT_GETPWNAM_R_AND_FRIENDS;             \
   INIT_CLOCK_GETTIME;                      \
   INIT_GETITIMER;                          \
   INIT_TIME;                               \
   INIT_GLOB;                               \

sanitizer_common/sanitizer_common_interceptors_scanf.inc

 //===-- sanitizer_common_interceptors_scanf.inc -----------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
-// Scanf implementation for use in *Sanitizer interceptors.
+// Scanf/printf implementation for use in *Sanitizer interceptors.
 // Follows http://pubs.opengroup.org/onlinepubs/9699919799/functions/fscanf.html
 // with a few common GNU extensions.
 //
 //===----------------------------------------------------------------------===//
 #include <stdarg.h>
 
 struct ScanfDirective {
   int argIdx; // argument index, or -1 of not specified ("%n$")
   int fieldWidth;
while (*p >= '0' && *p <= '9')
     ++p;
   return p;
 }
 
 static bool char_is_one_of(char c, const char *s) {
   return !!internal_strchr(s, c);
 }
 
-// Parse scanf format string. If a valid directive in encountered, it is
+// Parse format string. If a valid directive in encountered, it is
 // returned in dir. This function returns the pointer to the first
 // unprocessed character, or 0 in case of error.
 // In case of the end-of-string, a pointer to the closing \0 is returned.
-static const char *scanf_parse_next(const char *p, bool allowGnuMalloc,
+static const char *format_parse_next(const char *p, bool allowGnuMalloc,
                                     ScanfDirective *dir) {
   internal_memset(dir, 0, sizeof(*dir));
   dir->argIdx = -1;
 
   while (*p) {
     if (*p != '%') {
       ++p;
       continue;
if (allowGnuMalloc && dir->convSpecifier == 'a' &&
       }
     }
     break;
   }
   return p;
 }
 
 // Returns true if the character is an integer conversion specifier.
-static bool scanf_is_integer_conv(char c) {
+static bool format_is_integer_conv(char c) {
   return char_is_one_of(c, "diouxXn");
 }
 
 // Returns true if the character is an floating point conversion specifier.
-static bool scanf_is_float_conv(char c) {
+static bool format_is_float_conv(char c) {
   return char_is_one_of(c, "aAeEfFgG");
 }
 
 // Returns string output character size for string-like conversions,
 // or 0 if the conversion is invalid.
-static int scanf_get_char_size(ScanfDirective *dir) {
+static int format_get_char_size(ScanfDirective *dir) {
   if (char_is_one_of(dir->convSpecifier, "CS")) {
     // wchar_t
     return 0;
   }
 
   if (char_is_one_of(dir->convSpecifier, "cs[")) {
     if (dir->lengthModifier[0] == 'l')
       // wchar_t
       return 0;
     else if (dir->lengthModifier[0] == 0)
       return sizeof(char);
     else
       return 0;
   }
 
   return 0;
 }
 
-enum ScanfStoreSize {
+enum FormatStoreSize {
   // Store size not known in advance; can be calculated as strlen() of the
   // destination buffer.
-  SSS_STRLEN = -1,
+  FSS_STRLEN = -1,
   // Invalid conversion specifier.
-  SSS_INVALID = 0
+  FSS_INVALID = 0
 };
 
-// Returns the store size of a scanf directive (if >0), or a value of
+// Returns the store size of a format directive (if >0), or a value of
 // ScanfStoreSize.
-static int scanf_get_store_size(ScanfDirective *dir) {
+static int format_get_value_size(ScanfDirective *dir) {
   if (dir->allocate) {
     if (!char_is_one_of(dir->convSpecifier, "cCsS["))
-      return SSS_INVALID;
+      return FSS_INVALID;
     return sizeof(char *);
   }
 
   if (dir->maybeGnuMalloc) {
     if (dir->convSpecifier != 'a' || dir->lengthModifier[0])
-      return SSS_INVALID;
+      return FSS_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)) {
+  if (format_is_integer_conv(dir->convSpecifier)) {
     switch (dir->lengthModifier[0]) {
     case 'h':
       return dir->lengthModifier[1] == 'h' ? sizeof(char) : sizeof(short);
     case 'l':
       return dir->lengthModifier[1] == 'l' ? sizeof(long long) : sizeof(long);
     case 'L':
       return sizeof(long long);
     case 'j':
       return sizeof(INTMAX_T);
     case 'z':
       return sizeof(SIZE_T);
     case 't':
       return sizeof(PTRDIFF_T);
     case 0:
       return sizeof(int);
     default:
-      return SSS_INVALID;
+      return FSS_INVALID;
     }
   }
 
-  if (scanf_is_float_conv(dir->convSpecifier)) {
+  if (format_is_float_conv(dir->convSpecifier)) {
     switch (dir->lengthModifier[0]) {
     case 'L':
     case 'q':
       return sizeof(long double);
     case 'l':
       return dir->lengthModifier[1] == 'l' ? sizeof(long double)
                                            : sizeof(double);
     case 0:
       return sizeof(float);
     default:
-      return SSS_INVALID;
+      return FSS_INVALID;
     }
   }
 
   if (char_is_one_of(dir->convSpecifier, "sS[")) {
-    unsigned charSize = scanf_get_char_size(dir);
+    unsigned charSize = format_get_char_size(dir);
     if (charSize == 0)
-      return SSS_INVALID;
+      return FSS_INVALID;
     if (dir->fieldWidth == 0)
-      return SSS_STRLEN;
+      return FSS_STRLEN;
     return (dir->fieldWidth + 1) * charSize;
   }
 
   if (char_is_one_of(dir->convSpecifier, "cC")) {
-    unsigned charSize = scanf_get_char_size(dir);
+    unsigned charSize = format_get_char_size(dir);
     if (charSize == 0)
-      return SSS_INVALID;
+      return FSS_INVALID;
     if (dir->fieldWidth == 0)
       return charSize;
     return dir->fieldWidth * charSize;
   }
 
   if (dir->convSpecifier == 'p') {
     if (dir->lengthModifier[1] != 0)
-      return SSS_INVALID;
+      return FSS_INVALID;
     return sizeof(void *);
   }
 
-  return SSS_INVALID;
+  return FSS_INVALID;
 }
 
 // Common part of *scanf interceptors.
 // Process format string and va_list, and report all store ranges.
 // Stops when "consuming" n_inputs input items.
 static void scanf_common(void *ctx, int n_inputs, bool allowGnuMalloc,
                          const char *format, va_list aq) {
   CHECK_GT(n_inputs, 0);
   const char *p = format;
 
+  COMMON_INTERCEPTOR_READ_RANGE(ctx, format, internal_strlen(format) + 1);
+
   while (*p) {
     ScanfDirective dir;
-    p = scanf_parse_next(p, allowGnuMalloc, &dir);
+    p = format_parse_next(p, allowGnuMalloc, &dir);
     if (!p)
       break;
     if (dir.convSpecifier == 0) {
       // This can only happen at the end of the format string.
       CHECK_EQ(*p, 0);
       break;
     }
     // Here the directive is valid. Do what it says.
     if (dir.argIdx != -1) {
       // Unsupported.
       break;
     }
     if (dir.suppressed)
       continue;
-    int size = scanf_get_store_size(&dir);
-    if (size == SSS_INVALID)
+    int size = format_get_value_size(&dir);
+    if (size == FSS_INVALID) {
+      Report("WARNING: unexpected format specifier in scanf interceptor\n");
       break;
+    }
     void *argp = va_arg(aq, void *);
     if (dir.convSpecifier != 'n')
       --n_inputs;
     if (n_inputs < 0)
       break;
-    if (size == SSS_STRLEN) {
+    if (size == FSS_STRLEN) {
       size = internal_strlen((const char *)argp) + 1;
     }
     COMMON_INTERCEPTOR_WRITE_RANGE(ctx, argp, size);
+    // FIXME: detect overlaps with format string?
+  }
+}
+
+// Common part of *printf interceptors.
+// Process format string and va_list, and report all load ranges.
+static void printf_common(void *ctx, const char *format, va_list aq) {
+  const char *p = format;
+
+  COMMON_INTERCEPTOR_READ_RANGE(ctx, format, internal_strlen(format) + 1);
+
+  while (*p) {
+    ScanfDirective dir;
+    p = format_parse_next(p, false, &dir);
+    if (!p)
+      break;
+    if (dir.convSpecifier == 0) {
+      // This can only happen at the end of the format string.
+      CHECK_EQ(*p, 0);
+      break;
+    }
+    // Here the directive is valid. Do what it says.
+    if (dir.argIdx != -1) {
+      // Unsupported.
+      break;
+    }
+    if (dir.suppressed)
+      continue;
+    int size = format_get_value_size(&dir);
+    if (size == FSS_INVALID) {
+      Report("WARNING: unexpected format specifier in printf interceptor\n");
+      break;
+    }
+    if (dir.convSpecifier == 'n') {
+      void *argp = va_arg(aq, void *);
+      COMMON_INTERCEPTOR_WRITE_RANGE(ctx, argp, size);
+      // FIXME: detect overlaps with format string?
+      continue;
+    } else if (size == FSS_STRLEN) {
+      void *argp = va_arg(aq, void *);
+      size = internal_strlen((const char *)argp) + 1;
+      COMMON_INTERCEPTOR_READ_RANGE(ctx, argp, size);
+    } else {
+      // Skip non-pointer args
+      switch (size) {
+      case 1:
+      case 2:
+      case 4:
+        va_arg(aq, u32);
+        break;
+      case 8:
+        va_arg(aq, u64);
+        break;
+      default:
+        CHECK(0);
+        break;
+      }
+    }
   }
 }

sanitizer_common/sanitizer_flags.h

struct CommonFlags {
   // Invoke leak checking in an atexit handler. Has no effect if
   // detect_leaks=false, or if __lsan_do_leak_check() is called before the
   // handler has a chance to run.
   bool leak_check_at_exit;
   // If false, the allocator will crash instead of returning 0 on out-of-memory.
   bool allocator_may_return_null;
   // If false, disable printing error summaries in addition to error reports.
   bool print_summary;
+  // Check printf arguments.
+  bool check_printf;
 };
 
 inline CommonFlags *common_flags() {
   static CommonFlags f;
   return &f;
 }
 
 void SetCommonFlagsDefaults(CommonFlags *f);
 void ParseCommonFlagsFromString(CommonFlags *f, const char *str);
 
 }  // namespace __sanitizer
 
 #endif  // SANITIZER_FLAGS_H

sanitizer_common/sanitizer_flags.cc

void SetCommonFlagsDefaults(CommonFlags *f) {
   f->handle_ioctl = false;
   f->malloc_context_size = 1;
   f->log_path = "stderr";
   f->verbosity = 0;
   f->detect_leaks = false;
   f->leak_check_at_exit = true;
   f->allocator_may_return_null = false;
   f->print_summary = true;
+  f->check_printf = true;
 }
 
 void ParseCommonFlagsFromString(CommonFlags *f, const char *str) {
   ParseFlag(str, &f->symbolize, "symbolize");
   ParseFlag(str, &f->external_symbolizer_path, "external_symbolizer_path");
   ParseFlag(str, &f->strip_path_prefix, "strip_path_prefix");
   ParseFlag(str, &f->fast_unwind_on_fatal, "fast_unwind_on_fatal");
   ParseFlag(str, &f->fast_unwind_on_malloc, "fast_unwind_on_malloc");
   ParseFlag(str, &f->handle_ioctl, "handle_ioctl");
   ParseFlag(str, &f->malloc_context_size, "malloc_context_size");
   ParseFlag(str, &f->log_path, "log_path");
   ParseFlag(str, &f->verbosity, "verbosity");
   ParseFlag(str, &f->detect_leaks, "detect_leaks");
   ParseFlag(str, &f->leak_check_at_exit, "leak_check_at_exit");
   ParseFlag(str, &f->allocator_may_return_null, "allocator_may_return_null");
   ParseFlag(str, &f->print_summary, "print_summary");
+  ParseFlag(str, &f->check_printf, "check_printf");
 
   // Do a sanity check for certain flags.
   if (f->malloc_context_size < 1)
     f->malloc_context_size = 1;
   if (!f->symbolize)
     f->external_symbolizer_path = "";
 }
 

sanitizer_common/sanitizer_platform_interceptors.h

 #define SANITIZER_INTERCEPT_PREADV64 SI_LINUX_NOT_ANDROID
 #define SANITIZER_INTERCEPT_PWRITEV64 SI_LINUX_NOT_ANDROID
 
 #define SANITIZER_INTERCEPT_PRCTL   SI_LINUX
 
 #define SANITIZER_INTERCEPT_LOCALTIME_AND_FRIENDS SI_NOT_WINDOWS
 #define SANITIZER_INTERCEPT_STRPTIME SI_NOT_WINDOWS
 
 #define SANITIZER_INTERCEPT_SCANF SI_NOT_WINDOWS
 #define SANITIZER_INTERCEPT_ISOC99_SCANF SI_LINUX
 
+#ifndef SANITIZER_INTERCEPT_PRINTF
+# define SANITIZER_INTERCEPT_PRINTF SI_NOT_WINDOWS
+# define SANITIZER_INTERCEPT_ISOC99_PRINTF SI_LINUX
+#endif
+
 #define SANITIZER_INTERCEPT_FREXP 1
 #define SANITIZER_INTERCEPT_FREXPF_FREXPL SI_NOT_WINDOWS
 
 #define SANITIZER_INTERCEPT_GETPWNAM_AND_FRIENDS SI_NOT_WINDOWS
 #define SANITIZER_INTERCEPT_GETPWNAM_R_AND_FRIENDS \
   SI_MAC || SI_LINUX_NOT_ANDROID
 #define SANITIZER_INTERCEPT_CLOCK_GETTIME SI_LINUX
 #define SANITIZER_INTERCEPT_GETITIMER SI_NOT_WINDOWS

tsan/lit_tests/printf-1.c

+// RUN: %clang_tsan -O2 %s -o %t
+// RUN: %t 2>&1 | FileCheck %s
+
+#include <stdio.h>
+int main() {
+  volatile char c = '0';
+  volatile int x = 12;
+  volatile char s[] = "34";
+  printf("%c %d %s\n", c, x, s);
+  return 0;
+  // Check that printf works fine under Tsan.
+  // CHECK: 0 12 34
+}

tsan/rtl/tsan_stat.h

enum StatType {
   StatInt_sscanf,
   StatInt_fscanf,
   StatInt___isoc99_vscanf,
   StatInt___isoc99_vsscanf,
   StatInt___isoc99_vfscanf,
   StatInt___isoc99_scanf,
   StatInt___isoc99_sscanf,
   StatInt___isoc99_fscanf,
+  StatInt_vprintf,
+  StatInt_vsprintf,
+  StatInt_vsnprintf,
+  StatInt_vasprintf,
+  StatInt_vfprintf,
+  StatInt_printf,
+  StatInt_sprintf,
+  StatInt_snprintf,
+  StatInt_asprintf,
+  StatInt_fprintf,
+  StatInt___isoc99_vprintf,
+  StatInt___isoc99_vsprintf,
+  StatInt___isoc99_vsnprintf,
+  StatInt___isoc99_vasprintf,
+  StatInt___isoc99_vfprintf,
+  StatInt___isoc99_printf,
+  StatInt___isoc99_sprintf,
+  StatInt___isoc99_snprintf,
+  StatInt___isoc99_asprintf,
+  StatInt___isoc99_fprintf,
   StatInt_on_exit,
   StatInt___cxa_atexit,
   StatInt_localtime,
   StatInt_localtime_r,
   StatInt_gmtime,
   StatInt_gmtime_r,
   StatInt_ctime,
   StatInt_ctime_r,

tsan/rtl/tsan_stat.cc

void StatOutput(u64 *stat) {
   name[StatInt_sscanf]                   = "  sscanf                          ";
   name[StatInt_fscanf]                   = "  fscanf                          ";
   name[StatInt___isoc99_vscanf]          = "  vscanf                          ";
   name[StatInt___isoc99_vsscanf]         = "  vsscanf                         ";
   name[StatInt___isoc99_vfscanf]         = "  vfscanf                         ";
   name[StatInt___isoc99_scanf]           = "  scanf                           ";
   name[StatInt___isoc99_sscanf]          = "  sscanf                          ";
   name[StatInt___isoc99_fscanf]          = "  fscanf                          ";
+  name[StatInt_vprintf]                  = "  vprintf                         ";
+  name[StatInt_vsprintf]                 = "  vsprintf                        ";
+  name[StatInt_vsnprintf]                = "  vsnprintf                       ";
+  name[StatInt_vasprintf]                = "  vasprintf                       ";
+  name[StatInt_vfprintf]                 = "  vfprintf                        ";
+  name[StatInt_printf]                   = "  printf                          ";
+  name[StatInt_sprintf]                  = "  sprintf                         ";
+  name[StatInt_snprintf]                 = "  snprintf                        ";
+  name[StatInt_asprintf]                 = "  asprintf                        ";
+  name[StatInt_fprintf]                  = "  fprintf                         ";
+  name[StatInt___isoc99_vprintf]         = "  vprintf                         ";
+  name[StatInt___isoc99_vsprintf]        = "  vsprintf                        ";
+  name[StatInt___isoc99_vsnprintf]       = "  vsnprintf                       ";
+  name[StatInt___isoc99_vfprintf]        = "  vfprintf                        ";
+  name[StatInt___isoc99_printf]          = "  printf                          ";
+  name[StatInt___isoc99_sprintf]         = "  sprintf                         ";
+  name[StatInt___isoc99_snprintf]        = "  snprintf                        ";
+  name[StatInt___isoc99_fprintf]         = "  fprintf                         ";
   name[StatInt_on_exit]                  = "  on_exit                         ";
   name[StatInt___cxa_atexit]             = "  __cxa_atexit                    ";
   name[StatInt_localtime]                = "  localtime                       ";
   name[StatInt_localtime_r]              = "  localtime_r                     ";
   name[StatInt_gmtime]                   = "  gmtime                          ";
   name[StatInt_gmtime_r]                 = "  gmtime_r                        ";
   name[StatInt_ctime]                    = "  ctime                           ";
   name[StatInt_ctime_r]                  = "  ctime_r                         ";