Check for functions calls in sizeof calculations by pfultz2 · Pull Request #1111 · cppcheck-opensource/cppcheck

pfultz2 · 2018-03-09T18:07:45Z

A common mistake that I have found in our codebase is calling a function to get an integer or enum that represents the type such as:

int numBytes = numElements * sizeof(x.GetType());

This extends cppcheck for sizeof calculation to check for function calls as well.

amai2012

Maybe I want to keep numElements return codes of GetType()? What's wrong with it? I don't see an obvious error...

Even if it would indicate an error one should introduce another message probably.

danmar · 2018-03-10T14:12:31Z

Thanks! Could you tweak the error message so it says something like "found function call within sizeof()". A separate error id would help also. Before this is accepted I want that we test this on real code and see how much noise it generates (I will test it with the daca2 script).

pfultz2 · 2018-03-11T22:12:55Z

Maybe I want to keep numElements return codes of GetType()?

What do you mean?

What's wrong with it? I don't see an obvious error...

Because GetType() returns an enum which represents the type such as float or double. In this case it will compute the number of bytes based on the size of the enum not on the size of float or double(which is the intention of the code). We see a lot of those errors(for example here).

pfultz2 · 2018-03-11T22:21:18Z

A separate error id would help also.

Sure, is there a dev document that explains how to add new ids?

Before this is accepted I want that we test this on real code and see how much noise it generates

I did a simple grep(with grep -E '\bsizeof$[^()"*]+\([^()]*$') over several codebases(like qtbase, llvm, cocos2d, rethinkdb, redis, and php-src) and found that most things were macros that did not do a function call. If cppcheck expands the macro it shouldn't be a problem. The only other issue was that it is common to use sizeof(f()) to create a type trait. Is there a way in cppcheck to detect if its in a template? Or perhaps the function it refers to is an overloaded function?

I will test it with the daca2 script

What is the daca2 script?

danmar · 2018-03-12T09:58:14Z

Sure, is there a dev document that explains how to add new ids?

No sadly not. But I think your solution is ok. Spontaneously I thought we would still have 1 check function but it could call both sizeofCalculationError() and sizeofFunctionError(). But if the logic is too different it's better that it is splitted.

most things were macros that did not do a function call. If cppcheck expands the macro it shouldn't be a problem. The only other issue was that it is common to use sizeof(f()) to create a type trait. Is there a way in cppcheck to detect if its in a template?

Maybe unexpanded macros could be a problem. It's normal that macro definitions are missing. you might want to ensure that it's really a function (the Token::function() is non-null, or _settings->library has function configuration). EDIT: I see, you use Token::function(), then unexpanded macros are not a problem.

There is no way now to detect if it is in a template. But we have Token::isTemplateArg() so you can check if the function return type depends on a template argument.

What is the daca2 script?

It's like your grep but a more powerful. The script runs latest Cppcheck on all debian source code (thousands of projects, 100+ GB of source code).

The results are shown here:

http://cppcheck.sourceforge.net/devinfo/daca2-report/daca2.html

If you want to look at some particular warnings and have a error id you want to look at you can use this cgi script (this takes several seconds to load):

http://cppcheck.sourceforge.net/cgi-bin/daca2-search.cgi?id=sizeofCalculation

danmar · 2018-03-12T10:12:08Z

I assume you will fix the test failure soonish:

test/testsizeof.cpp:113: Assertion failed.
Expected:
[test.cpp:1]: (warning) Calling 'sizeof' on 'sizeof'.\n

Actual:
[test.cpp:1]: (warning) Calling 'sizeof' on 'sizeof'.\n
[test.cpp:1]: (warning) Found function call inside sizeof().\n

danmar · 2018-03-12T10:25:44Z

I believe the code looks pretty ok. if the test is fixed I will apply this and then the daca2 script can test it.

amai2012 · 2018-03-12T11:14:42Z

Maybe I want to keep numElements return codes of GetType()?

What do you mean?

What's wrong with it? I don't see an obvious error...
Because GetType() returns an enum which represents the type such as float or double. In this case it will compute the number of bytes based on the size of the enum not on the size of float or double(which is the intention of the code). We see a lot of those errors(for example here).

Would that change trigger a warning on

#include <cstdlib>
short foo() {
  return 1;
}
int main() {
  const int NUM=42;
  short* buffer = (short*)malloc(NUM*sizeof foo() );
  for (int i=0; i<NUM; ++i)
  {
     buffer[i] = foo();
  }
  return 0;
}

? That would be a false positive IMHO.

Even the small example from the PR

int foo() { return 1; }; int a,sizeof(sizeof(foo()))

is a message I don't want to see at all.

danmar · 2018-03-12T13:36:02Z

Would that change trigger a warning on

You have of course simplified your code. But it looks weird to me; why not use sizeof(short) instead in such cases? Are you talking about generic code (expanded macro/template) that ends up in this? Or do you see sizeof(foo()) in normal code?

Even the small example from the PR

Yes as far as I see in the code, the intention seems to be to only report the "Calling 'sizeof' on 'sizeof'." warning and not warn about the function call.

pfultz2 · 2018-03-12T15:15:29Z

Would that change trigger a warning on

Yes it will, but grepping over qtbase, llvm, cocos2d, rethinkdb, redis, and php-src, I have only seen two cases for this. So its not very common.

pfultz2 · 2018-03-12T15:27:15Z

Yes as far as I see in the code, the intention seems to be to only report the "Calling 'sizeof' on 'sizeof'." warning and not warn about the function call.

Actually that's not really the case. The code bails on sizeof because Token::match will match the inner sizeof. I have updated the codebase and removed the faulty test case.

I also found a problem with code like this:

template<class T>
struct A
{
    static B f(const B &);
    static A f(const A &);
    static A &g();
    static T &h();

    enum {
        X = sizeof(f(g() >> h())) == sizeof(A),
        Y = sizeof(f(g() << h())) == sizeof(A),
        Z = X & Y
    };
};

It seems like cppcheck cant resolve the function f. And then the code moves to g which is not overloaded, so it then reports an error. For now, I have it stop at the first eName found, so it won't report an error for this case.

danmar · 2018-03-13T09:13:53Z

It seems like cppcheck cant resolve the function f.

Hmm it is not conclusive how f should be resolved as far as I see. The type of the >> and << depends on how those are overloaded. However it is the convention that the result type for bit shifts is the same as the LHS type.

And then the code moves to g which is not overloaded, so it then reports an error. For now, I have it stop at the first eName found, so it won't report an error for this case.

It is a bit weird implementation. it is preferable to look at the ast tree instead of walking the token list. I would suggest that you only look at the astOperand2() for the "(" token.

 if (Token::simpleMatch(tok, "sizeof ("))
     const Token *checkToken = tok->next()->astOperand2();

danmar · 2018-03-13T09:40:23Z

Unfortunately, our ValueType handling has gaps, it does not always succeed. If you see some problems that is very interesting and should be fixed if possible. There are many checkers that rely on ValueType.

pfultz2 · 2018-03-13T12:53:34Z

It is a bit weird implementation. it is preferable to look at the ast tree instead of walking the token list.

How do you walk the ast? I didn't see any ast matchers.

I would suggest that you only look at the astOperand2() for the "(" token.

But I don't know if this would catch the x.foo() case?

pfultz2 · 2018-03-13T12:54:22Z

Unfortunately, our ValueType handling has gaps, it does not always succeed. If you see some problems that is very interesting and should be fixed if possible. There are many checkers that rely on ValueType.

Is this meant for PR #1114?

danmar · 2018-03-13T13:10:14Z

How do you walk the ast? I didn't see any ast matchers.

We don't have it. I would like to have it someday.

But I don't know if this would catch the x.foo() case?

It would.

It would not catch sizeof(1+foo()) but then you have a "calculation in sizeof()" warning instead.

danmar

some more stylistic nits

danmar · 2018-03-14T09:58:49Z

+              "{\n"
+              "    int bar() { return 1; };\n"
+              "}\n"
+              "Foo f;int a,sizeof(f.bar())");


this test code is weird. it is not valid syntax. how about replacing int a,sizeof(f.bar()) with int a=sizeof(f.bar());.

danmar · 2018-03-14T10:16:01Z

+
+        check("#define foo() int\n"
+              "int a,sizeof(foo())");
+        ASSERT_EQUALS("", errout.str());


this is not technically correct. The testcases are not preprocessed. so I believe that during testing the checker would see # define foo ( ) int .... But when you actually run it on this code, it will not see the define at all. Then the checker will just see sizeof ( int ).

Imho you can just remove the first line and assert that the checker will not warn when a sizeof(foo()) is seen and it does not know what foo is.

danmar · 2018-03-14T10:19:35Z

+
+        check("#define M(x) sizeof(x)\n"
+              "int foo() { return 1; }; int a,M(foo())");
+        ASSERT_EQUALS("", errout.str());


this is not properly tested because we don't run the preprocessor in the tests. you can write a test code and indicate that certain tokens are expanded macro(s) using the special character $.

int a = $sizeof $( foo() $) ;

that would mean that the Token::isExpandedMacro() would be true for the outer "sizeof (" and ")" tokens. And it would be false for foo().

danmar · 2018-03-14T10:31:22Z

+void CheckSizeof::sizeofFunctionError(const Token *tok, bool inconclusive)
+{
+    reportError(tok, Severity::warning,
+                "sizeofFunction", "Found function call inside sizeof().", CWE682, inconclusive);


I think the id sizeofFunctionCall would be a bit better

danmar · 2018-03-14T10:33:42Z

+            }
+
+            for (const Token *argument = tok->next()->astOperand2(); argument; argument = argument->astOperand2()) {
+                const Token *checkToken = argument->previous();


why do you have a loop, is there a test case where it is not enough to only look at the first token?

Yes, when doing sizeof(x.foo()), but let me double check.

You can rewrite that as ’sizeof((x.foo)())’. Then you can intuitively see that the operands for ’.’ are x and foo.

You are right, I was doing something else wrong. I updated to just check the first token.

danmar · 2018-03-14T10:35:56Z

+    }
+}
+
+void CheckSizeof::sizeofFunctionError(const Token *tok, bool inconclusive)


The inconclusive argument can be removed, it's always false. Unless you plan to start using it someday.

amai2012 · 2018-03-15T08:12:25Z

Would that change trigger a warning on
You have of course simplified your code. But it looks weird to me; why not use sizeof(short) instead in such cases? Are you talking about generic code (expanded macro/template) that ends up in this? Or do you see sizeof(foo()) in normal code?

I think that quite reasonable generic code could be written that way.
A quick scan on my disc however reveals no examples except in boost.

Even the small example from the PR
Yes as far as I see in the code, the intention seems to be to only report the "Calling 'sizeof' on 'sizeof'." warning and not warn about the function call.

Yes, that intention I would support.

pfultz2 · 2018-03-15T08:30:13Z

I think that quite reasonable generic code could be written that way.
A quick scan on my disc however reveals no examples except in boost.

A common case is to call an overloaded function in order to build a type trait. However, I avoid this case by making sure the function is not overloaded.

danmar · 2018-03-15T09:18:33Z

I think that quite reasonable generic code could be written that way.

A generic code example (invented, not real code):

template<class T>
struct Foo {
    auto foo1(T t) -> decltype(t + 1L) { return t + 1L; }
    void foo2() { int x = sizeof(foo1()); }
};
Foo<int> intfoo;
Foo<float> floatfoo;

I doubt that ValueType determines the foo1 return type properly now. But the checker should not warn for such code.

Well I think that when daca2 is running, we will see lots of real test cases.

danmar · 2018-03-15T09:27:09Z

I merged this. Now daca2 will be running this checker. Results from this checker will be available here:

http://cppcheck.sourceforge.net/cgi-bin/daca2-search.cgi?id=sizeofFunctionCall

The first results could come in a few hours.

pfultz2 · 2018-03-15T20:30:34Z

I merged this. Now daca2 will be running this checker. Results from this checker will be available here:

How long does this take? I checked and there still aren't any results.

danmar · 2018-03-15T20:49:41Z

I would guess that tomorrow evening it has run through all the code.

We can already see that this is not noisy.

It is not a problem for me if it doesn't catch many bugs.

danmar · 2018-03-18T06:33:24Z

Still no results. Weird. One explanation is that in the daca2 script, no include paths are set and therefore the function pointer will often be NULL. Maybe we could start trying to set the include paths.

pfultz2 · 2018-03-18T23:07:48Z

I have gotten results from some codebases without using include. Maybe those codebases are not part of daca2. I am not sure why there isn't any results.

Check for functions calls in sizeof calculations

e884886

amai2012 reviewed Mar 10, 2018

View reviewed changes

pfultz2 added 7 commits March 11, 2018 18:37

Use seperate message and id for sizeofFunction

80ebaa5

Check for overloads

2c3f437

Using decltype with a function should not be an error

fe2ac5a

Fix warning

fe86bed

Fix false positives when running pass the close paren

0e01813

Fix test error

0047c1b

Try to fix more false positives

243d263

Traverse using astOperand2

18ebe8b

danmar requested changes Mar 14, 2018

View reviewed changes

pfultz2 added 2 commits March 14, 2018 23:27

Only check first argument

6ee752d

Update fixes from feedback from PR

9d5c3fd

danmar merged commit 166e4ca into cppcheck-opensource:master Mar 15, 2018

pfultz2 deleted the sizeof-call branch May 19, 2019 05:25

Conversation

pfultz2 commented Mar 9, 2018

Uh oh!

amai2012 left a comment

Choose a reason for hiding this comment

Uh oh!

danmar commented Mar 10, 2018

Uh oh!

pfultz2 commented Mar 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pfultz2 commented Mar 11, 2018

Uh oh!

danmar commented Mar 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danmar commented Mar 12, 2018

Uh oh!

danmar commented Mar 12, 2018

Uh oh!

amai2012 commented Mar 12, 2018

Uh oh!

danmar commented Mar 12, 2018

Uh oh!

pfultz2 commented Mar 12, 2018

Uh oh!

pfultz2 commented Mar 12, 2018

Uh oh!

danmar commented Mar 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danmar commented Mar 13, 2018

Uh oh!

pfultz2 commented Mar 13, 2018

Uh oh!

pfultz2 commented Mar 13, 2018

Uh oh!

danmar commented Mar 13, 2018

Uh oh!

danmar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danmar Mar 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amai2012 commented Mar 15, 2018

Uh oh!

pfultz2 commented Mar 15, 2018

Uh oh!

danmar commented Mar 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danmar commented Mar 15, 2018

Uh oh!

pfultz2 commented Mar 15, 2018

Uh oh!

danmar commented Mar 15, 2018

Uh oh!

danmar commented Mar 18, 2018

Uh oh!

pfultz2 commented Mar 18, 2018

Uh oh!

Reviewers

pfultz2 commented Mar 11, 2018 •

edited

Loading

danmar commented Mar 12, 2018 •

edited

Loading

danmar commented Mar 13, 2018 •

edited

Loading

danmar Mar 14, 2018 •

edited

Loading

danmar commented Mar 15, 2018 •

edited

Loading