This is an archive of the discontinued LLVM Phabricator instance.

[analyzer][StdLibraryFunctionsChecker] Elaborate the summary of fread and fwrite
ClosedPublic

Authored by martong on Sep 3 2020, 4:29 AM.

Details

Summary

Add the BufferSize argument constraint to fread and fwrite. This change
itself makes it possible to discover a security critical case, described
in SEI-CERT ARR38-C.

We also add the not-null constraint on the 3rd arguments.

In this patch, I also remove those lambdas that don't take any
parameters (Fwrite, Fread, Getc), thus making the code better
structured.

Diff Detail

Event Timeline

martong created this revision.Sep 3 2020, 4:29 AM
martong requested review of this revision.Sep 3 2020, 4:29 AM

This checker will make an additional assumption on fread and fwrite with the ReturnValueCondition. The return value is constrained by StreamChecker too but it splits the error (if returned value is less that arg 3) and non-error cases into separate branches. I think this causes no problem because it will refine the assumption made here (if this assumption is made first) or the assumption here has no effect (if the split happened already).

This checker will make an additional assumption on fread and fwrite with the ReturnValueCondition.

There is nothing new in that. This assumption described by the .Case has been here since the inception of this Checker. This patch does not change it. This patch adds two new argument constraints.

The return value is constrained by StreamChecker too but it splits the error (if returned value is less that arg 3) and non-error cases into separate branches. I think this causes no problem because it will refine the assumption made here (if this assumption is made first) or the assumption here has no effect (if the split happened already).

Either way, this is not a problem. However, in a similar case with the CallAndMessage Checker, we decided to list the more specific Checker as a dependency. We could do that with StreamChecker too, if you think that's better that way. But I'd rather keep that as it is now, since as you suggests, it works now and will work even after this patch.

The patch looks great, in fact, it demonstrates how well thought out your summary crafting machinery is.

However, in a similar case with the CallAndMessage Checker, we decided to list the more specific Checker as a dependency.

We got the answer to D77061#2057063! We should turn it into a weak dependency though (D80905).

This checker will make an additional assumption on fread and fwrite with the ReturnValueCondition. The return value is constrained by StreamChecker too but it splits the error (if returned value is less that arg 3) and non-error cases into separate branches. I think this causes no problem because it will refine the assumption made here (if this assumption is made first) or the assumption here has no effect (if the split happened already).

Be sure to triple check whether the ExplodedGraph looks okay with both checkers enabled.

The patch looks great, in fact, it demonstrates how well thought out your summary crafting machinery is.

However, in a similar case with the CallAndMessage Checker, we decided to list the more specific Checker as a dependency.

We got the answer to D77061#2057063! We should turn it into a weak dependency though (D80905).

This checker will make an additional assumption on fread and fwrite with the ReturnValueCondition. The return value is constrained by StreamChecker too but it splits the error (if returned value is less that arg 3) and non-error cases into separate branches. I think this causes no problem because it will refine the assumption made here (if this assumption is made first) or the assumption here has no effect (if the split happened already).

Be sure to triple check whether the ExplodedGraph looks okay with both checkers enabled.

I'll try to create tests that check the state in both order.

martong updated this revision to Diff 291606.Sep 14 2020, 10:14 AM
  • Add tests to verify compatibility of the two checkers
Szelethus accepted this revision.Sep 14 2020, 10:39 PM

@balazske may have some closing words.

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
1316

differant->different

This revision is now accepted and ready to land.Sep 14 2020, 10:39 PM
balazske accepted this revision.Sep 15 2020, 2:55 AM

There is a blatant regression we introduced in D87240. Actually, the test added here could have catched that regression.
See https://reviews.llvm.org/D87240#inline-812062
I am putting back the modeling dependency with this patch.

Actually, the test added here could have catched that regression.

Correction: It is the new BufferSize argument constraint in fread's summary and the existing test test_fread_uninitialized in std-c-library-functions.c that could have catched that regression. (Not the test in std-c-library-functions-vs-stream-checker.c.)

Post-commit LGTM on the post-commit changes!