Confusing control flow analysis from Parasoft C++test

Question

We use Parasoft C++test to statically analyze our code. It's having trouble with code like the following:

void foo(int* x) {
    try {
        bar();
    } catch(...) {
        delete x;
        throw;
    }

    *x;
}

It warns on the *x; line that:

Freed memory shouldn't be subsequently accessed under any circumstances

Somehow it's concluded that control flow can pass into the catch(...) block, delete x, go past the throw;, and make it to *x;. I tried throw std::exception(""); and a couple others and got the same thing. Parasoft certainly knows about exceptions and incorporates them into its control flow, because there are many other tests that involve exception checking. Is it just confused in this case, or is there actually some way for the execution of this program to hit both delete x; and *x;?

I'd say it is confused - but you should ask Parasoft. I'd also say that if you have real code like this, you are doing things wrong. — , Apr 26 '11 at 20:39
Can you extract the `delete x;` into an inline method? Or does Parasoft know to look in there also? — quamrana, Apr 26 '11 at 20:40
@unapersson It was just a snippet that demonstrated the problem — Michael Mrozek, Apr 26 '11 at 20:40
You could try adding an `abort()` right after the `throw`. But who knows, perhaps that will be diagnosed as dead code ... — Peter G., Apr 26 '11 at 21:04
It might inspect bar(); If bar() is known to free x (or x is statically known to be freed before entering foo) this wouldn't be wrong. In all other cases, the flow analysis is confused :) — sehe, Apr 26 '11 at 21:10
@PeterG Yeah, the compiler is more intelligent and flags it. Which, come to think of it, is a pretty good indication that Parasoft is wrong. It's possible to `#ifdef` out the `abort()` so only Parasoft sees it, but I'd rather just suppress the error message as long as I know it's invalid — Michael Mrozek, Apr 26 '11 at 21:11
@sehe `bar()` is actually empty, and removing the call doesn't change the analysis (I only included it to try and avoid it detecting that the `catch` would never happen, but I guess it doesn't check that) — Michael Mrozek, Apr 26 '11 at 21:12
Just fishing now, but did you try to actually throw some other exception within the catch block? Like maybe a `throw std::exception("Hi");` Perhaps it doesn't know how to deal with a re-throw? — Chris A., Apr 26 '11 at 21:25
Actually, for humour, have any of you looked at the Parasoft website? Cool buzzword madness! I guess they ain't selling to techies any more, — , Apr 26 '11 at 21:31
@Chris I did, I had a line about it ("I tried `throw std::exception("");` and a couple others and got the same thing"). I tried `std::exception`, `std::runtime_error`, a custom class, and just `throw 0;`; they all behaved the same. It sounds like it just botches `throw` in general; I tried it outside a `catch` block (just `delete x; throw 0; *x;`) and it still complained that `*x` was dereferencing freed memory, without realizing it was dead — Michael Mrozek, Apr 26 '11 at 21:43

score 2 · Answer 1 · answered May 02 '11 at 18:10

So the tool is wrong (It's been said before, I know), and I assume you dont want to swith of the warning.

I agree with @Pascal's comment that it is somewhat dangerous to rewrite code just to work around some tool's limitiations. What you can do is disable this warining for just the files where you currently have this problem.

Then, you have a warning free build to begin with, without a tool telling to rewrite existing valid code.

For new code, you will have to adopt some style that's understood by the tool. This is less of an issue because that will be code you currently work on, so it would be less of an issue if you need to rewrite it slightly to get rid of warnings.

Although correct, the existing style is far from ideal.

I would reccomend storing pointers like x in auto_ptr's. That will automatically delete the content of the auto_ptr if it goes out of scope - unless you explicitly take it out of the auto_ptr. This is a lot easier on the eyes, and also nicely documents that this function takes ownership of the pointer.

void foo(auto_ptr<int> x)
{
    bar();
    *x;
}

I would expect ParaSoft will have no problems with this code.

score 0 · Answer 2 · answered Jun 03 '13 at 22:43

Quick update:

1) The above mentioned rule is not a flow analysis rule at all. That being said, the rule (ID MRM-31) was improved in C++test 9.2.0 and later. There is also a corresponding flow analysis rule for this in C++test (ID: BD-RES-FREE) that should do what you want.

score 0 · Answer 3 · answered Apr 27 '11 at 14:46

0

Perhaps this is a daft suggestion, but what does Parasoft say if you leave the catch for the end? I.e.

void foo(int* x) 
  {
  try 
    {
    bar();
    *x;
    } 
  catch(...) 
    {
    delete x;
    throw;
    }      
  }

I realize that may not work for all combinations of statements and exceptions, for example if you have multiple exception types to catch with different handling at different stages of foo, but at least it may give you the start of a workaround if you really want to get rid of the warning.

answered Apr 27 '11 at 14:46

Joris Timmermans

10,814
2
49
75

1

If it works around the problem it's not really beside the point - though I do tend to favor just ignoring the obviously wrong conclusion by Parasoft's analysis tool. – Joris Timmermans Apr 27 '11 at 15:23
I second the comment. If you start working around false alarms by changing the code, you will introduce bugs. It may be one time in 50 or just one time in 200. It doesn't matter: you are spending time introducing bugs. – Pascal Cuoq Apr 27 '11 at 20:27

Confusing control flow analysis from Parasoft C++test

3 Answers3