Clang plugin matcher for code that should use SprintfLiteral

RESOLVED FIXED in Firefox 52

Status

defect
P3
normal
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: evilpie, Assigned: evilpie)

Tracking

(Blocks 1 bug)

unspecified
mozilla52
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(2 attachments, 5 obsolete attachments)

Posted patch Match snprintf with sizeof (obsolete) — Splinter Review
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 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
Priority: -- → P3
Assignee: nobody → evilpies
Attachment #8790351 - Attachment is obsolete: true
Depends on: 1302233
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 :)
(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, ...);
Added additional handling of 'const int x = 10' like good. Improved error message for vsnprintf.
Attachment #8790446 - Attachment is obsolete: true
Attachment #8790494 - Flags: review?(michael)
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?)
Attachment #8790494 - Flags: review?(michael)
Thanks, I applied your feedback mostly verbatim.
Attachment #8790494 - Attachment is obsolete: true
Attachment #8791345 - Flags: review?(michael)
Comment on attachment 8791345 [details] [diff] [review]
v2 - Show an error for {v}snprintf with literals

Letting ehsan do the final review :).
Attachment #8791345 - Flags: review?(michael) → review?(ehsan)
(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 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/cfe-commits@lists.llvm.org/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.
Attachment #8791345 - Flags: review?(ehsan) → review-
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.
Attachment #8791345 - Attachment is obsolete: true
Attachment #8793496 - Flags: review?(ehsan)
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.
Attachment #8793496 - Flags: review?(ehsan) → review+
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.
Attachment #8793822 - Flags: review?(ehsan)
(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 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>.
Attachment #8793822 - Flags: review?(ehsan) → review-
(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!  :)
(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?
Flags: needinfo?(nfroyd)
(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.
Flags: needinfo?(nfroyd)
(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.
Attachment #8793822 - Attachment is obsolete: true
Attachment #8797631 - Flags: review?(ehsan)
Attachment #8797631 - Flags: review?(ehsan) → review+
Pushed by evilpies@gmail.com:
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
https://hg.mozilla.org/mozilla-central/rev/b66170ee0000
https://hg.mozilla.org/mozilla-central/rev/628c7aa9630f
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.