Page MenuHomePhabricator

[Checkers] Added support for freopen to StreamChecker.
ClosedPublic

Authored by balazske on Nov 7 2019, 7:15 AM.

Details

Summary

Extend StreamChecker with a new evaluation function for API call 'freopen'.

Event Timeline

balazske created this revision.Nov 7 2019, 7:15 AM

Thank you for working on this. Please add comments to the code and maybe also to the tests. I could not find anything in the standards about freopen() with null-pointer for the stream parameter.

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
220

The four lines above are typical cases for using auto, since the type is duplicated in the line. See: https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable

264

Please use a more elaborated error message here.

clang/test/Analysis/stream.c
120

I wonder if this test belongs into this particular patch. It has nothing to do with freopen().

From the description The original stream (if it exists) is closed. I think it is possible that the original stream does "not exist". But a test program crashed with NULL argument (but not with a closed file). So null argument is not permitted or at least does not work always.

balazske updated this revision to Diff 228424.Nov 8 2019, 5:46 AM
balazske marked an inline comment as done.
  • Do not allow null stream to freopen.
  • Added comments.
balazske marked 2 inline comments as done.Nov 8 2019, 5:57 AM

I am still not sure in the auto type, I did not see that way of auto usage often in clang code.

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
220

This is not totally clear if we can trust the pattern that at auto X = Y.getXxx() type of X will be Xxx.

clang/test/Analysis/stream.c
120

I wanted to see if "aliasing" works because it happens in the freopen tests too. Probably this test is not needed?

NoQ added inline comments.Nov 13 2019, 1:06 PM
clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
220

@balazske +1. I'm personally in favor of using auto here, but the community's stance is to use auto only for dyn_cast/getAs etc. calls where type is explicitly spelled out, and also for iterators where the type is going to be too long and annoying to spell out (but in the latter case i'm suddenly in favor of spelling it out, as i always get confused about how many *s do i need to add in order to properly dereference whatever's in there).

233–234

That's actually one more good reason to move all checkNullStream to PreCall: less spaghetti. The same checker should feel free to subscribe for the same function in multiple different callbacks and do different things in them.

Rule of a thumb (i'm not sure if it's always applicable but it's applicable to most "typical" checkers even if they aren't currently written this way):

  1. Pre-condition checking goes in PreCall;
  2. Modeling that affects everybody goes in evalCall;
  3. Checker's internal bookkeeping goes in PostCall.
236–248

It looks like you're creating a symbol only to assume that it's null. You should bind a concrete null (i.e., loc::ConcreteInt) instead.

252

The null StreamVal is going to be caught by checkNullStream(). What are the other possibilities here? Concrete FILE * that isn't null?

I guess it's possible but it probably deserves a comment describing that this is actually a very rare scenario. Maybe a test that involves a value like ((FILE *)0x12345).

263–264

What does really happen when you freopen a stream that isn't already open? It sounds like it's doing a fclose blindly, so we might as well report a double close in this case, and then we don't need to update the program state because it's already opened(?) Though later we still might want to update the program state if we start keeping track of the filename or the mode.

balazske updated this revision to Diff 229268.Nov 14 2019, 4:00 AM
  • Simplified the code.
balazske marked 5 inline comments as done.Nov 14 2019, 4:07 AM
balazske added inline comments.
clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
233–234

Precondition (check for NULL stream) can go into PreCall. Probably the bookkeeping task is more difficult to do, if the state is split we have already the different states available in evalCall but need to check the state in postCall.

263–264

The documentation says that the stream is closed but error is ignored. So the double close should be no problem. If the file was already open the state should not be changed (the set call handles that case and does not generate new state?).

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
70

I would move this freopen line and everything else (evalFreopen header and body) directly after freopen. To me it seems more logical.

220

SValBuilder is explicitly spelled out here, also DefinedSVal below, which comes from a getAs(). Btw, is it right to call the actual variable SValBuilder as well? I would never do that. I is usually called SVB.

233–234

@NoQ +1 I completely agree: All precondition checks in this checker should be moved into PreCall. I would even go further: separate the checker into a StreamModeling which models the calls using evalCall and StreamChecker which does the checks. However, these NFCs should be done in subsequent patches, not this one.

balazske updated this revision to Diff 230076.Nov 19 2019, 8:03 AM
balazske marked an inline comment as done.
  • Moved freopen after fopen, removed 'SValBuilder'.
balazske marked 3 inline comments as done.Nov 19 2019, 8:14 AM
balazske added inline comments.
clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
233–234

Does the setting of new state (State->set<StreamMap>) belong to the "modeling" (evalCall) or "checker" (in the postCall) part?

NoQ added inline comments.Nov 19 2019, 3:45 PM
clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
245

Mmm, wait, this doesn't look right to me. You cannot deduce from the presence of freopen() in the code that the argument may be null, so the split over the argument is not well-justified.

The right thing to do here will be to produce a "Schrödinger file descriptor" that's both Opened and OpenFailed until we observe the return value to be constrained to null or non-null later during analysis (cf. D32449), otherwise conservatively assume that the open succeeded when queried for the first time (because that's how non-paranoid code under analysis will behave).

Or you could drop the failed path immediately if the argument isn't known to be zero. That'll drop the coverage slightly but won't cause anything bad to happen.

balazske marked an inline comment as done.Nov 20 2019, 1:56 AM
balazske added inline comments.
clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
245

Where does this comment belong? I got this in the email:

Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:201
+    std::tie(StateRetNotNull, StateRetNull) =
+        CM.assumeDual(StateStreamNull, RetVal);
+    if (StateRetNull) {

Is the problem with the null-check of argument of freopen? Why is this different than the other calls where checkNullStream is used?
Or is the problem with the case of NULL return value from freopen (in this case the argument was non-null, for the null case we assume program crash)? In this case we really do not know what happened with the file descriptor argument (but the value of it is not changed), so the code assumes now that is will be OpenFailed. This is not accurate always (it may remain open or become closed). We could have another state split here (for these cases)?

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
206

I think it would be better to rearrange these lines to exactly match evalFopen().

245

The argument is checked against NULL, becuase it must not be NULL. That is the "checker" part. The return value is split like in the existing code for modeling fopen(). I do not see anything wrong here.

martong added inline comments.
clang/test/Analysis/stream.c
160

This comment is confusing for me. Maybe there are typos here?
Isn't the 'freopen' failed? fopen was successful and it's return value was checked in line 153.
f1 seems to be ok here (still associated with "foo.c"), only f2 is bad, as we see in line 162.

Or f1 is already closed here, even though it could not open the new file?

If a new filename is specified, the function first attempts to close any file already associated with stream (third parameter) and disassociates it. Then, independently of whether that stream was successfuly closed or not, freopen opens the file specified by filename and associates it with the stream just as fopen would do using the specified mode.

https://stackoverflow.com/questions/20908740/check-the-return-value-of-freopen-in-c

balazske updated this revision to Diff 231905.Dec 3 2019, 7:04 AM
balazske marked an inline comment as done.
  • rearrangement in evelFreopen, updated comment in test
balazske added inline comments.Dec 3 2019, 7:14 AM
clang/test/Analysis/stream.c
160

The freopen has failed, not the fopen. I think we can not know if f1 points to any file after this, or is in closed state (but we know that is not safe to use). Probably both the close and the re-open can fail.

balazske marked an inline comment as done.Dec 3 2019, 7:16 AM
martong accepted this revision.Dec 3 2019, 7:23 AM

LGTM, but wait for others please.

This revision is now accepted and ready to land.Dec 3 2019, 7:23 AM
This revision was automatically updated to reflect the committed changes.
NoQ added inline comments.Dec 5 2019, 5:54 PM
clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
245

The final code looks correct to me, thanks!~