Page MenuHomePhabricator

[clang-tidy] Add new checker for comparison with runtime string functions.
ClosedPublic

Authored by etienneb on Apr 1 2016, 11:06 AM.

Details

Summary

This checker is validating suspicious usage of string compare functions.

Example:

if (strcmp(...))       // Implicitly compare to zero
if (!strcmp(...))      // Won't warn
if (strcmp(...) != 0)  // Won't warn

This patch was checked over large amount of code.
There is three checks:

  • Implicit comparator to zero (coding-style, many warnings found),
  • Suspicious implicit cast to non-integral (bugs!?, almost none found),
  • Comparison to suspicious constant (bugs!?, found two cases),

Example:
https://github.com/kylepjohnson/sigma/blob/master/sigma/native-installers/debian/dependencies/files/opt/sigma/E/HEURISTICS/che_to_precgen.c

else if(strcmp(id, "select") == 0)
{
   array->array[i].key1 = 25;
}
else if(strcmp(id, "sk") == 28)      // BUG!?
{
   array->array[i].key1 = 20;
}

Diff Detail

Event Timeline

etienneb updated this revision to Diff 52397.Apr 1 2016, 11:06 AM
etienneb retitled this revision from to [clang-tidy] Add new checker for comparison with runtime string functions..
etienneb updated this object.
etienneb added a reviewer: alexfh.
etienneb added a subscriber: cfe-commits.
etienneb updated this object.Apr 1 2016, 11:07 AM

Implicit conversions to bool should be handled by readability-implicit-bool-cast.

As pointed by Eugene,

The following code seems to produce multiple errors.

int foo() {
  if (strcmp(A, "a") == true)
    return 0;
  return 1;
}

Results:

/home/etienneb/examples/test.cc:56:7: warning: function 'strcmp' is compared to a suspicious constant [misc-suspicious-string-compare]
  if (strcmp(A, "a") == true)
      ^
/home/etienneb/examples/test.cc:56:25: warning: implicit cast bool -> 'int' [readability-implicit-bool-cast]
  if (strcmp(A, "a") == true)
                        ^
/home/etienneb/examples/test.cc:56:25: warning: redundant boolean literal supplied to boolean operator [readability-simplify-boolean-expr]
  if (strcmp(A, "a") == true)
                        ^
      static_cast<bool>(strcmp(A, "a"))

On my side, I'm not against to keep it that way.
The implicit cast bool -> 'int' warning is often too noisy and is often ignored by programmers.
The other message sounds more specific and may ring a bell.

WDYT?

Eugene.Zelenko added inline comments.Apr 1 2016, 11:55 AM
docs/clang-tidy/checks/misc-suspicious-string-compare.rst
13

I don't know documentation markup well, but looks like you need to add ".. code-block:: c++" before code snippet. See performance-implicit-cast-in-loop for example.

As pointed by Eugene,

The following code seems to produce multiple errors.

int foo() {
  if (strcmp(A, "a") == true)
    return 0;
  return 1;
}

Results:

/home/etienneb/examples/test.cc:56:7: warning: function 'strcmp' is compared to a suspicious constant [misc-suspicious-string-compare]
  if (strcmp(A, "a") == true)
      ^
/home/etienneb/examples/test.cc:56:25: warning: implicit cast bool -> 'int' [readability-implicit-bool-cast]
  if (strcmp(A, "a") == true)
                        ^
/home/etienneb/examples/test.cc:56:25: warning: redundant boolean literal supplied to boolean operator [readability-simplify-boolean-expr]
  if (strcmp(A, "a") == true)
                        ^
      static_cast<bool>(strcmp(A, "a"))

On my side, I'm not against to keep it that way.
The implicit cast bool -> 'int' warning is often too noisy and is often ignored by programmers.
The other message sounds more specific and may ring a bell.

WDYT?

Actually I meant "if (strcmp(...))" statement when I refer to readability-implicit-bool-cast.

You're right, both are outputted.

/home/etienneb/examples/test.cc:56:7: warning: function 'strcmp' is called without explicitly comparing result [misc-suspicious-string-compare]
  if (strcmp(A, "a"))
      ^
                     != 0
/home/etienneb/examples/test.cc:56:7: warning: implicit cast 'int' -> bool [readability-implicit-bool-cast]
  if (strcmp(A, "a"))
      ^
                     != 0
etienneb updated this revision to Diff 52420.Apr 1 2016, 1:48 PM

Fix support for C.
Adding test for C99 code.

etienneb updated this revision to Diff 52421.Apr 1 2016, 1:53 PM
etienneb marked an inline comment as done.

nits and formatting

Eugene, I double checked and the implicit bool version doesn't trigger in C.
This check is applied in C and C++.
(note: I needed to add code to fix C)

docs/clang-tidy/checks/misc-suspicious-string-compare.rst
14

Any idea how to validate the look after formatting?

alexfh added inline comments.Apr 1 2016, 9:29 PM
clang-tidy/misc/SuspiciousStringCompareCheck.cpp
39

Should we add a configuration option to support custom string compare functions (e.g. lstrcmp)?

docs/clang-tidy/checks/misc-suspicious-string-compare.rst
14
  1. Install sphinx:

    $ sudo apt-get install sphinx-common
  1. Enable LLVM_BUILD_DOCS and maybe some other options in cmake.
  1. ninja docs-clang-tools-html (or something similar, if you use make).
test/clang-tidy/misc-suspicious-string-compare.cpp
120

Please remove "[misc-suspicious-string-compare]" (and maybe some more static) from all but the first CHECK-MESSAGES lines.

alexfh requested changes to this revision.Apr 1 2016, 9:29 PM
alexfh edited edge metadata.
This revision now requires changes to proceed.Apr 1 2016, 9:29 PM

I spent time thinking about Eugene.Zelenko@ comment and I start to believe that given the difference between the errors ratio and the warnings ratio, maybe we should gate the report 'explicitly comparing result' under a configurable option.

There is an order of magnitude more readability-implicit-bool-cast than string-compare-explicitly-comparing-result.
There is an order of magnitude more string-compare-explicitly-comparing-result than compared-to-a-suspicious-constant.

So, the compared-to-a-suspicious-constant check should always be on because there is almost no false-positive.
The string compare with implicit comparator should be gated under a option.
And the user can decide to activate or not the check readability-implicit-bool-cast.

clang-tidy/misc/SuspiciousStringCompareCheck.cpp
39

I like the idea. But, I don't expect many projects to add custom configuration. So, if you are aware of any string compare variants I'm missing, please share them.

I didn't know about ' lstrcmp'. So many variants.

I think will be good idea to convert !strcmp(...) to strcmp(...) == 0.

etienneb updated this revision to Diff 52601.Apr 4 2016, 12:36 PM
etienneb edited edge metadata.
etienneb marked 3 inline comments as done.

add options, configurable list of function names, support for ! pattern.

etienneb updated this revision to Diff 52602.Apr 4 2016, 12:40 PM
etienneb edited edge metadata.

nits

alexfh added inline comments.Apr 7 2016, 8:37 AM
clang-tidy/misc/SuspiciousStringCompareCheck.cpp
74

I'd initialize the list as a comma/semicolon-separated list in a single string.

120

The loop is not needed. I guess, you just can use the proper hasAnyName overload.

219

getName() is not needed.

231

!= nullptr is not commonly used in clang/clang-tidy code.

alexfh requested changes to this revision.Apr 7 2016, 9:16 AM
alexfh edited edge metadata.
This revision now requires changes to proceed.Apr 7 2016, 9:16 AM
etienneb updated this revision to Diff 53966.Apr 15 2016, 3:45 PM
etienneb edited edge metadata.

alexfh comments.

ok, alexfh.
I addressed your comments.

etienneb marked 4 inline comments as done.Apr 15 2016, 8:07 PM

ship draft.

etienneb updated this revision to Diff 53978.Apr 15 2016, 8:08 PM
etienneb edited edge metadata.

nits

alexfh accepted this revision.Apr 19 2016, 4:03 PM
alexfh edited edge metadata.

Looks good with a couple of nits. Thank you!

clang-tidy/misc/SuspiciousStringCompareCheck.cpp
26

The variable is not const right now. I recently started preferring the constexpr char X[] = ... way of defining string constants.

109

nit: FuntionNames -> FunctionNames

This revision is now accepted and ready to land.Apr 19 2016, 4:03 PM
etienneb updated this revision to Diff 54526.Apr 21 2016, 10:13 AM
etienneb marked 2 inline comments as done.
etienneb edited edge metadata.

rebased

etienneb updated this revision to Diff 54531.Apr 21 2016, 10:24 AM

fix integration

etienneb closed this revision.Apr 21 2016, 10:25 AM