1302163 - Clang plugin matcher for code that should use SprintfLiteral

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect, P3)

Description

[Tom S [:evilpie]]

Attachment: Match snprintf with sizeof

SnprintfLiteral should be used instead of doing something like snprintf(x, sizeof(x), ...) or Y x[100]; snprintf(x, 100, ...). I started working on a patch that matches the first kind, but I am going to look into matching the second case as well.

Currently this results in unfixable errors, because we match something in nsprpub/pr/src/io/prprf.c, which is C code and thus can't use SprintfLiteral.

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8790351 [details] [diff] [review]
Match snprintf with sizeof

Review of attachment 8790351 [details] [diff] [review]:
-----------------------------------------------------------------

Please also add a test before you try to land this.

I'll file a bug about us running the clang plugin on nsprpub.

::: build/clang-plugin/clang-plugin.cpp
@@ +38,5 @@
> +// #define cxxConstructExpr constructExpr
> +// #define cxxConstructorDecl constructorDecl
> +// #define cxxMethodDecl methodDecl
> +// #define cxxNewExpr newExpr
> +// #define cxxRecordDecl recordDecl

Why did you comment these out?

@@ +867,5 @@
>        && Method->getName() == AssertName;
>  }
>  
> +AST_MATCHER(CallExpr, isPrintfLikeFunc) {
> +  static const std::string Name1 = "snprintf";

Can this be "Snprintf" or similar? Name1 is not the nicest name.

@@ +873,5 @@
> +  const FunctionDecl *Method = Node.getDirectCallee();
> +
> +  return Method
> +      && Method->getDeclName().isIdentifier()
> +      && (Method->getName() == Name1 || Method->getName() == Name2);

Please use getNameChecked()

@@ +1869,5 @@
> +
> +  // const Expr *E = FuncCall->getArg(1);
> +  // if (!isa<sizeOfExpr>(E)) {
> +  //   return;
> +  // }

Remove this commented out code

Comment 2

[Tom S [:evilpie]]

Attachment: Show an error for snprintf with literals

Comment 3

[Tom S [:evilpie]]

I added a check for IntegerLiteral, but I really want to check for a constant integer value, in case that makes a difference in this context.

We should probably add something similar for {v}sprintf, or just disallow those completely :)

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to Tom Schuster [:evilpie] from comment #3)
> I added a check for IntegerLiteral, but I really want to check for a
> constant integer value, in case that makes a difference in this context.

I think you have to be slightly more sophisticated, because I'm pretty sure one can have:

const size_t /* or similar type */ sz = 100;
char buf[sz];

snprintf(buf, sz, ...);

Comment 5

[Tom S [:evilpie]]

Attachment: Show an error for {v}snprintf with literals

Added additional handling of 'const int x = 10' like good. Improved error message for vsnprintf.

Comment 6

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8790494 [details] [diff] [review]
Show an error for {v}snprintf with literals

Review of attachment 8790494 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/clang-plugin/clang-plugin.cpp
@@ +291,4 @@
>    return false;
>  }
>  
> +bool isIgnoredPathForString(const Decl *Declaration) {

Can we call this "isIgnoredPathForSprintfChecker" or similar? isIgnoredPathForString doesn't clearly communicate what checker this is related to to me.

In addition, this function is never called within this patch, so I don't know why it is here.

@@ +891,5 @@
>  
> +AST_MATCHER(CallExpr, isSnprintfLikeFunc) {
> +  static const std::string Snprintf = "snprintf";
> +  static const std::string Vsnprintf = "vsnprintf";
> +  const FunctionDecl *Method = Node.getDirectCallee();

I find it strange that we have a function named `Method`, can we call this `Func` or something like that?

@@ +919,5 @@
> +        Begin->compare_lower(StringRef("mtransport")) == 0 ||
> +        Begin->compare_lower(StringRef("protobuf")) == 0 ||
> +        Begin->compare_lower(StringRef("skia")) == 0 ||
> +        Begin->compare_lower(StringRef("testing")) == 0 ||
> +        Begin->compare_lower(StringRef("updater")) == 0) {

Can we move these checks into an "isInIgnoredPathFor" method? They feel out of place in the AST_MATCHER and bloat the size of the function.

@@ +1928,5 @@
> +  DiagnosticsEngine &Diag = Result.Context->getDiagnostics();
> +  unsigned SizeofError = Diag.getDiagnosticIDs()->getCustomDiagID(
> +    DiagnosticIDs::Error, "%0 called with sizeof should use %1 instead.");
> +  unsigned ConstantError = Diag.getDiagnosticIDs()->getCustomDiagID(
> +    DiagnosticIDs::Error, "%0 called with constant should use %1 instead.");

I'm not a super big fan of these error messages. How about "Use %1 instead of %0 to write into a fixed size buffer"? I'm not sure how much I like that message either, but I feel like it might be more clear?

@@ +1962,5 @@
> +      // Match calls like: const int y = 100; snprintf(x, y, ...);
> +      Literal = Result.Nodes.getNodeAs<IntegerLiteral>("constant");
> +    }
> +
> +    if (Type->getSize() == Literal->getValue()) {

I feel like we should complain if the literal is larger than Type->getSize too, but with a different error message. I'm not sure if that is within the scope of this patch though.

(Do we hit that case anywhere in M-C?)

Comment 7

[Tom S [:evilpie]]

Attachment: v2 - Show an error for {v}snprintf with literals

Thanks, I applied your feedback mostly verbatim.

Comment 8

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8791345 [details] [diff] [review]
v2 - Show an error for {v}snprintf with literals

Letting ehsan do the final review :).

Comment 9

[Tom S [:evilpie]]

Looks like this doesn't build on try https://treeherder.mozilla.org/logviewer.html#?job_id=27493496&repo=try :/

Comment 10

[Nika Layzell [:nika] (ni? for response)]

(In reply to Tom Schuster [:evilpie] from comment #9)
> Looks like this doesn't build on try
> https://treeherder.mozilla.org/logviewer.html#?job_id=27493496&repo=try :/

It looks like you might be using matchers which are not available in all versions of clang which we support. I haven't looked at the error message enough to be sure.

The version of clang used in try is specified here: http://searchfox.org/mozilla-central/source/build/build-clang/clang-static-analysis-linux64.json

Comment 11

[(no longer active)]

Comment on attachment 8791345 [details] [diff] [review]
v2 - Show an error for {v}snprintf with literals

Review of attachment 8791345 [details] [diff] [review]:
-----------------------------------------------------------------

Minusing because this doesn't build and the version checks done in the patch look wrong.

If you want to test your changes against the clang we use to build, you can use tooltool to download it and test locally.  See <https://wiki.mozilla.org/ReleaseEngineering/Applications/Tooltool>.  You probably want to do something like:

tooltool.py fetch -m browser/config/tooltool-manifests/linux64/clang.manifest

The tooltool manifest we're using clang 3.8 now per bug 1262781.

::: build/clang-plugin/clang-plugin.cpp
@@ +31,4 @@
>  typedef ASTConsumer *ASTConsumerPtr;
>  #endif
>  
> +#if !defined(HAVE_NEW_ASTMATCHER_NAMES) && CLANG_VERSION_FULL < 309

Please do not add clang version checks.  Instead, if you're relying on a new matcher (and it looks like you are based on the try failure) you should add a configure check for it and make sure the code builds with and without that feature.

@@ +35,4 @@
>  // In clang 3.8, a number of AST matchers were renamed to better match the
>  // respective AST node.  We use the new names, and #define them to the old
>  // ones for compatibility with older versions.
> +// Clang 3.9 doesn't define HAVE_NEW_ASTMATCHER_NAMES anymore.

I don't understand what this comment is talking about.  Clang has never defined this macro.  We define it here based on this configure check: <http://searchfox.org/mozilla-central/source/build/autoconf/clang-plugin.m4#121>

@@ +306,5 @@
> +        Begin->compare_lower(StringRef("libstagefright")) == 0 ||
> +        Begin->compare_lower(StringRef("mtransport")) == 0 ||
> +        Begin->compare_lower(StringRef("protobuf")) == 0 ||
> +        Begin->compare_lower(StringRef("skia")) == 0 ||
> +        Begin->compare_lower(StringRef("testing")) == 0 ||

Why testing?

@@ +307,5 @@
> +        Begin->compare_lower(StringRef("mtransport")) == 0 ||
> +        Begin->compare_lower(StringRef("protobuf")) == 0 ||
> +        Begin->compare_lower(StringRef("skia")) == 0 ||
> +        Begin->compare_lower(StringRef("testing")) == 0 ||
> +        Begin->compare_lower(StringRef("updater")) == 0) {

Why updater?

@@ +1251,5 @@
> +
> +  AstMatcher.addMatcher(
> +      callExpr(isSnprintfLikeFunc(),
> +        allOf(hasArgument(0, ignoringParenImpCasts(declRefExpr().bind("buffer"))),
> +                             anyOf(hasArgument(1, sizeOfExpr(has(ignoringParenImpCasts(declRefExpr().bind("size"))))),

If I were to make a wild guess, I'd say your code is hitting <https://www.mail-archive.com/[e-mail]/msg25234.html>.

@@ +1907,5 @@
> +  }
> +
> +  DiagnosticsEngine &Diag = Result.Context->getDiagnostics();
> +  unsigned ErrorID = Diag.getDiagnosticIDs()->getCustomDiagID(
> +    DiagnosticIDs::Error, "Use %1 instead of %0 to write into a fixed size buffer");

Buffers which aren't literal strings can also be fixed size.  I think a better error message would be:

"Use %1 instead of %0 when writing into a character array."

Also, since it's pretty unclear at first why the compiler is complaining, please append a note like this as well:

"This will prevent passing in the wrong size to %0 accidentally."

@@ +1915,5 @@
> +  StringRef Name = D->getDirectCallee()->getName();
> +  const char *Replacement;
> +  if (Name == "snprintf") {
> +    Replacement = "SprintfLiteral";
> +  } else {

Please check Name here as well, in the interest of keeping the check future compatible.

@@ +1941,5 @@
> +      // Match calls like: const int y = 100; snprintf(x, y, ...);
> +      Literal = Result.Nodes.getNodeAs<IntegerLiteral>("constant");
> +    }
> +
> +    if (Type->getSize() == Literal->getValue()) {

Hmm, do we really want to silence the warning for this case for example?

char buf[100];
const int size = 101; // I don't know much programming!
snprintf(buf, size, ...);

Please make this a <= check, and also add a test case like above.

Comment 12

[Tom S [:evilpie]]

Attachment: v3 - Show an error for [v]snprintf with literals

I (wrongly) assumed that the standalone? updater couldn't include "mozilla/Sprintf", but apparently it can. "testing" folders usually include a copy of gtest, which use a macro with snprintf.

Comment 13

[(no longer active)]

Comment on attachment 8793496 [details] [diff] [review]
v3 - Show an error for [v]snprintf with literals

Review of attachment 8793496 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great, r=me with the below fixed.  Thanks!

::: build/clang-plugin/clang-plugin.cpp
@@ +42,5 @@
>  #define cxxRecordDecl recordDecl
>  #endif
>  
> +#ifndef HAS_ACCEPTS_IGNORINGPARENIMPCASTS
> +#define hasIgnoringParenImpCasts has

For consistency, please change this to:

#define hasIgnoringParenImpCasts(x) has(x)

@@ +1954,5 @@
> +    }
> +
> +    if (Type->getSize().ule(Literal->getValue())) {
> +      Diag.Report(D->getLocStart(), ErrorID) << Name << Replacement;
> +      Diag.Report(D->getLocStart(), NoteID) << Name << Replacement;

NoteID only needs one replacement argument.

::: build/clang-plugin/tests/TestSprintfLiteral.cpp
@@ +5,5 @@
> +  snprintf(x, sizeof(x), "bar"); // expected-error {{Use SprintfLiteral instead of snprintf when writing into a character array.}} expected-note {{This will prevent passing in the wrong size to snprintf accidentally.}}
> +  snprintf(x, 100, "bar"); // expected-error {{Use SprintfLiteral instead of snprintf when writing into a character array.}} expected-note {{This will prevent passing in the wrong size to snprintf accidentally.}}
> +  snprintf(x, 101, "bar"); // expected-error {{Use SprintfLiteral instead of snprintf when writing into a character array.}} expected-note {{This will prevent passing in the wrong size to snprintf accidentally.}}
> +  const int hundred = 100;
> +  snprintf(x, hundred, "bar"); // expected-error {{Use SprintfLiteral instead of snprintf when writing into a character array.}} expected-note {{This will prevent passing in the wrong size to snprintf accidentally.}}

For completeness, please add a hundredandone test as well.  :-)

@@ +26,5 @@
> +  vsnprintf(x, sizeof(x), "bar", args); // expected-error {{Use VsprintfLiteral instead of vsnprintf when writing into a character array.}} expected-note {{This will prevent passing in the wrong size to vsnprintf accidentally.}}
> +  vsnprintf(x, 100, "bar", args); // expected-error {{Use VsprintfLiteral instead of vsnprintf when writing into a character array.}} expected-note {{This will prevent passing in the wrong size to vsnprintf accidentally.}}
> +  vsnprintf(x, 101, "bar", args); // expected-error {{Use VsprintfLiteral instead of vsnprintf when writing into a character array.}} expected-note {{This will prevent passing in the wrong size to vsnprintf accidentally.}}
> +  const int hundred = 100;
> +  vsnprintf(x, hundred, "bar", args); // expected-error {{Use VsprintfLiteral instead of vsnprintf when writing into a character array.}} expected-note {{This will prevent passing in the wrong size to vsnprintf accidentally.}}

Ditto.

Comment 14

[Tom S [:evilpie]]

Attachment: Change code to use SprintfLiteral instead of snprintf

This should be totally safe. However from what I remember on Windows snprintf doesn't null terminate when string len = buffer size, so this might be something to watch out for.

Comment 15

[(no longer active)]

(In reply to Tom Schuster [:evilpie] from comment #14)
> This should be totally safe. However from what I remember on Windows
> snprintf doesn't null terminate when string len = buffer size, so this might
> be something to watch out for.

MSDN disagrees <https://msdn.microsoft.com/en-us/library/2ts7cx93.aspx>:

"The snprintf function truncates the output when len is greater than or equal to count, by placing a null-terminator at buffer[count-1]."

Comment 16

[(no longer active)]

Comment on attachment 8793822 [details] [diff] [review]
Change code to use SprintfLiteral instead of snprintf

Review of attachment 8793822 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/src/XPCDebug.cpp
@@ -22,5 @@
> -  _vsnprintf(buffer, sizeof(buffer), fmt, ap);
> -#else
> -  vsnprintf(buffer, sizeof(buffer), fmt, ap);
> -#endif
> -  buffer[sizeof(buffer)-1] = '\0';

_vsnprintf on Windows seems to do what you suspected it to, so you should restore this line.

::: netwerk/sctp/datachannel/DataChannel.cpp
@@ -193,5 @@
>  
>    if (MOZ_LOG_TEST(gSCTPLog, LogLevel::Debug)) {
>      va_start(ap, format);
> -#ifdef _WIN32
> -    if (vsnprintf_s(buffer, sizeof(buffer), _TRUNCATE, format, ap) > 0) {

This is changing the meaning of this code.  See <https://msdn.microsoft.com/en-us/library/d3xd30zz.aspx>.

Comment 17

[Nathan Froyd [:froydnj]]

(In reply to :Ehsan Akhgari from comment #15)
> (In reply to Tom Schuster [:evilpie] from comment #14)
> > This should be totally safe. However from what I remember on Windows
> > snprintf doesn't null terminate when string len = buffer size, so this might
> > be something to watch out for.
> 
> MSDN disagrees <https://msdn.microsoft.com/en-us/library/2ts7cx93.aspx>:
> 
> "The snprintf function truncates the output when len is greater than or
> equal to count, by placing a null-terminator at buffer[count-1]."

snprintf on Windows used to not follow the standard in this respect, so you are both correct!  :)

Comment 18

[(no longer active)]

(In reply to Nathan Froyd [:froydnj] from comment #17)
> (In reply to :Ehsan Akhgari from comment #15)
> > (In reply to Tom Schuster [:evilpie] from comment #14)
> > > This should be totally safe. However from what I remember on Windows
> > > snprintf doesn't null terminate when string len = buffer size, so this might
> > > be something to watch out for.
> > 
> > MSDN disagrees <https://msdn.microsoft.com/en-us/library/2ts7cx93.aspx>:
> > 
> > "The snprintf function truncates the output when len is greater than or
> > equal to count, by placing a null-terminator at buffer[count-1]."
> 
> snprintf on Windows used to not follow the standard in this respect, so you
> are both correct!  :)

When did the behavior change?

Comment 19

[Nathan Froyd [:froydnj]]

(In reply to :Ehsan Akhgari from comment #18)
> (In reply to Nathan Froyd [:froydnj] from comment #17)
> > (In reply to :Ehsan Akhgari from comment #15)
> > > (In reply to Tom Schuster [:evilpie] from comment #14)
> > > > This should be totally safe. However from what I remember on Windows
> > > > snprintf doesn't null terminate when string len = buffer size, so this might
> > > > be something to watch out for.
> > > 
> > > MSDN disagrees <https://msdn.microsoft.com/en-us/library/2ts7cx93.aspx>:
> > > 
> > > "The snprintf function truncates the output when len is greater than or
> > > equal to count, by placing a null-terminator at buffer[count-1]."
> > 
> > snprintf on Windows used to not follow the standard in this respect, so you
> > are both correct!  :)
> 
> When did the behavior change?

Sorry, bad memory, MSVC used to not provide snprintf at all, according to:

https://hg.mozilla.org/mozilla-central/file/bdb8d05f8870/mfbt/Snprintf.h#l17

I don't recall whether _snprintf did the right thing or not.

Comment 20

[Tom S [:evilpie]]

(In reply to :Ehsan Akhgari from comment #16)
> Comment on attachment 8793822 [details] [diff] [review]
> Change code to use SprintfLiteral instead of snprintf
> 
> Review of attachment 8793822 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/xpconnect/src/XPCDebug.cpp
> @@ -22,5 @@
> > -  _vsnprintf(buffer, sizeof(buffer), fmt, ap);
> > -#else
> > -  vsnprintf(buffer, sizeof(buffer), fmt, ap);
> > -#endif
> > -  buffer[sizeof(buffer)-1] = '\0';
> 
> _vsnprintf on Windows seems to do what you suspected it to, so you should
> restore this line.
> 
> ::: netwerk/sctp/datachannel/DataChannel.cpp
> @@ -193,5 @@
> >  
> >    if (MOZ_LOG_TEST(gSCTPLog, LogLevel::Debug)) {
> >      va_start(ap, format);
> > -#ifdef _WIN32
> > -    if (vsnprintf_s(buffer, sizeof(buffer), _TRUNCATE, format, ap) > 0) {
> 
> This is changing the meaning of this code.  See
> <https://msdn.microsoft.com/en-us/library/d3xd30zz.aspx>.

I somehow doubt people actually want the MSVC behavior, but sure.

Comment 21

[Tom S [:evilpie]]

Attachment: v2 - Change code to use SprintfLiteral instead of snprintf

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b66170ee0000
Show an error for [v]snprintf with literals using the clang plugin. r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/628c7aa9630f
Change code to use SprintfLiteral instead of snprintf. r=ehsan

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b66170ee0000
https://hg.mozilla.org/mozilla-central/rev/628c7aa9630f