Page MenuHomePhabricator

[clang] Fortify warning for scanf calls with field width too big.
ClosedPublic

Authored by mbenfield on Oct 14 2021, 1:25 PM.

Diff Detail

Event Timeline

mbenfield created this revision.Oct 14 2021, 1:25 PM
mbenfield requested review of this revision.Oct 14 2021, 1:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 14 2021, 1:25 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
enh added a comment.Oct 14 2021, 2:30 PM

"shut up and take my money!" :-)

clang/lib/Sema/SemaChecking.cpp
678

(stray tabs?)

mbenfield added inline comments.Oct 15 2021, 9:41 AM
clang/lib/Sema/SemaChecking.cpp
678

Not sure what you're referring to. AFAICT there are no tabs in this file.

enh added inline comments.Oct 15 2021, 9:53 AM
clang/lib/Sema/SemaChecking.cpp
678

oh, gerrit uses the >> symbol to mean "tab". is this tool just saying "indentation changed", not specifically "someone added a tab"?

mbenfield added inline comments.Oct 15 2021, 2:30 PM
clang/lib/Sema/SemaChecking.cpp
678

Ah I see. Yes, I think that's correct, that it doesn't mean a tab here.

LGTM % nits -- thanks for this! :)

clang/include/clang/Basic/DiagnosticSemaKinds.td
836

nit: s/Warning </Warning</

838

nit: please s/null/NUL/ for consistency with elsewhere

clang/lib/Sema/SemaChecking.cpp
415

optional: llvm generally prefers FunctionRefs for simplicity and speed. if it's easy to refactor to use those (seems like it may be), please do. otherwise, it's not a big deal.

This revision is now accepted and ready to land.Oct 19 2021, 6:25 PM
mbenfield updated this revision to Diff 380956.Oct 20 2021, 7:59 AM

respond to comments: null to NUL, remove stray space, use function_ref

mbenfield marked 5 inline comments as done.Oct 20 2021, 8:01 AM

LGTM. Thanks again!

clang/lib/Sema/SemaChecking.cpp
749

nit: const auto if possible (and below)

770

Please put this in a variable and pass that into H's constructor. function_ref doesn't own the function it points to, so any use of this at line >= 758 (e.g., line 768) is a use-after-free.

rebase and rerun tests

mbenfield updated this revision to Diff 382802.Oct 27 2021, 2:34 PM

rebase and rerun tests

mbenfield updated this revision to Diff 382840.Oct 27 2021, 4:16 PM

fix function_ref use-after-free

mbenfield marked 2 inline comments as done.Oct 27 2021, 4:22 PM

This doesn't seem to be working very well:

thakis@thakis:~/src/llvm-project$ cat test.cc
#include <inttypes.h>
#include <stdio.h>
#include <stdint.h>

int main() {
  uint16_t hextets[8];
  int chars_scanned;
  char buf[] = "1234:5678:9abc:def0:1234:5678:9abc:def0";
  sscanf(buf,
	 "%4" SCNx16 ":%4" SCNx16 ":%4" SCNx16 ":%4" SCNx16 ":%4" SCNx16
	 ":%4" SCNx16 ":%4" SCNx16 ":%4" SCNx16 "%n",
	 &hextets[0], &hextets[1], &hextets[2], &hextets[3], &hextets[4],
	 &hextets[5], &hextets[6], &hextets[7], &chars_scanned);

  for (int i = 0; i < 8; ++i)
    printf("%x ", hextets[i]);
  printf("%d\n", chars_scanned);
}
thakis@thakis:~/src/llvm-project$ out/gn/bin/clang test.cc -Wall
test.cc:9:3: warning: 'sscanf' may overflow; destination buffer in argument 9 has size 4, but the corresponding field width plus NUL byte is 5 [-Wfortify-source]
  sscanf(buf,
  ^
test.cc:9:3: warning: 'sscanf' may overflow; destination buffer in argument 10 has size 2, but the corresponding field width plus NUL byte is 5 [-Wfortify-source]
2 warnings generated.
thakis@thakis:~/src/llvm-project$ ./a.out 
1234 5678 9abc def0 1234 5678 9abc def0 39
  1. The warning is emitted twice, but doesn't point at code the 2nd time round
  2. That code looks correct to me (ie there shouldn't be any warnings), maybe %n isn't handled correctly?
  3. The diag points at the start of the scanf instead of at the faulty arg.

Especially 2 is breaking builds, so I'll revert this for now. Looks like a cool warning though, looking forward to the relanding :)

mbenfield reopened this revision.Oct 28 2021, 9:31 AM
This revision is now accepted and ready to land.Oct 28 2021, 9:31 AM
mbenfield updated this revision to Diff 383064.Oct 28 2021, 9:31 AM

Only diagnose if conversion specifier is %s.

Give the location of the particular argument rather than the location of the function call.

Test to make sure we don't warn on %d.

Thanks for reverting. Here's another try.

This just shouldn't be warning on any specifier other than %s, so I fixed that.

As far as not pointing at code for the second warning, I verified manually that it does now point at code even when warning twice, but it doesn't seem to be feasible to include a test for that.

enh accepted this revision.Oct 28 2021, 9:41 AM

add tests for %c and %[ too? (it's genuinely unclear to me from a quick skim whether this patch covers them.)

mbenfield updated this revision to Diff 383161.Oct 28 2021, 2:10 PM

Support %c and %[ specifiers.

Changed the diagnostic message to accommodate.

mbenfield added a comment.EditedOct 28 2021, 2:10 PM

Previously this patch did not cover %c and %[, but now it does.

enh added a comment.Oct 28 2021, 5:12 PM

Previously this patch did not cover %c and %[, but now it does.

thanks!

mbenfield reopened this revision.Wed, Nov 3, 7:35 AM
This revision is now accepted and ready to land.Wed, Nov 3, 7:35 AM
mbenfield updated this revision to Diff 384431.Wed, Nov 3, 7:37 AM

Ignore specifiers with *.