This is an archive of the discontinued LLVM Phabricator instance.

[clang][Analyzer] Add errno state to standard functions modeling.
ClosedPublic

Authored by balazske on May 11 2022, 9:37 AM.

Details

Summary

This updates StdLibraryFunctionsChecker to set the state of 'errno'
by using the new errno_modeling functionality.
The errno value is set in the PostCall callback. Setting it in call::Eval
did not work for some reason and then every function should be
EvalCallAsPure which may be bad to do. Now the errno value and state
is not allowed to be checked in any PostCall checker callback because
it is unspecified if the errno was set already or will be set later
by this checker.

Diff Detail

Event Timeline

balazske created this revision.May 11 2022, 9:37 AM
Herald added a project: Restricted Project. · View Herald Transcript
balazske requested review of this revision.May 11 2022, 9:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 11 2022, 9:37 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Nice work! Could you pleas add some lit tests that describe an errno related bugreport for a standard lib function?

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

This class serves as a base class, it is not really a Kind class which is usually just an enum.
I'd prefer to call this ErrnoConstraintBase or BasicErrnoConstraint.

396

This class is a polymorphic base class, since we have this virtual function here. Would make sense to make destructor virtual as well, even though delete is not called explicitly on the base class (as I see).

410

Please document the subclasses.

642–644

Would it make sense to have a ErrnoIrrelevant as the default value for ErrnoC?

758–761

Could you please add some more documentation to these variables? And they could be const.

The bugreports look promising. However, I think we desperately need a note that describes which function has set the errno_modeling state. Below I'd expect the following notes for the highlighted function call.

  • assuming return value of mkdir is in range [0, INT_MAX]
  • errno is not set

I suppose, you'd need to add some changes for these both in the ErrnoChecker and here as well.

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

Please check if State can be nullptr.

Function mkdir is modeled incorrectly by the checker. According to the man page it can return 0 or -1 only (-1 is error) but the checker allows non-negative value at success. So the shown bug report is incorrect (it can be only -1 if not 0 and then check of errno is allowed). Anyway the note tags should be added to every function.

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

These strings are for test purposes only.

@martong Do you mean that a "describe" function is needed for the return value constraint of the function and for the errno constraint type? Then a note tag can contain what the function is assumed to return on success and what is allowed (or not) to do with errno. For example: "Assuming that 'mkdir' is successful, it returns zero in this case and value of 'errno' is unspecified after the call".

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

I think here it is never null. A relation is created between a new conjured symbol and zero, this can never fail, or not? (The ErrnoSVal should be here a newly created symbol. If the Tag is not used it may be the same symbol that was previously bound to the expression if EvalCallAsPure is used.)

642–644

It would be a bit more convenient but in the current design it is not possible to pass the member variable as default value. ErrnoIrrelevant is really used in less than half of the cases.

Function mkdir is modeled incorrectly by the checker. According to the man page it can return 0 or -1 only (-1 is error) but the checker allows non-negative value at success. So the shown bug report is incorrect (it can be only -1 if not 0 and then check of errno is allowed). Anyway the note tags should be added to every function.

Okay, could you please correct the summary then?

@martong Do you mean that a "describe" function is needed for the return value constraint of the function and for the errno constraint type? Then a note tag can contain what the function is assumed to return on success and what is allowed (or not) to do with errno. For example: "Assuming that 'mkdir' is successful, it returns zero in this case and value of 'errno' is unspecified after the call".

Yes, that sounds good. We have the virtual describe function in the base constraint class that could be used to explain what values we expect.

martong added inline comments.May 17 2022, 1:36 AM
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
448

Okay, I see your reasoning. However, we might never know how the constraint solver reasons about it, thus we should not rely on that. As a contrived example, it might check recursively that the LHS of the BinOp you just conjured and it might recognize that the State is infeasible.

642–644

Ok.

balazske updated this revision to Diff 431652.May 24 2022, 5:35 AM

added comments. added note tag, added test, improved the code

steakhal added inline comments.May 24 2022, 7:11 AM
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
406
425

The explicit qualification is not necessary. There are other cases like this.

In the constraint manager, we tend to use Op for operator kinds, it would be nice to use the same taxonomy here.

430
443–446

Just use .castAs<NonLoc>(). You will assert it anyway, and I find this much more readable.

454–460

I believe you should use Twine for this.

1024

What is <unknown> in this context? Please add a test for it or remove this branch.

1033–1038

Why did you change these?

clang/test/Analysis/errno-stdlibraryfunctions.c
21–25
29
balazske updated this revision to Diff 431909.May 25 2022, 1:53 AM
balazske marked 13 inline comments as done.

update

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
454–460

It should work, FunctionName.str() and getOpcodeStr and describeErrnoCheckState should return non-temporary strings.

balazske edited the summary of this revision. (Show Details)May 25 2022, 1:54 AM
balazske updated this revision to Diff 431924.May 25 2022, 2:55 AM
balazske edited the summary of this revision. (Show Details)

fixed some problems

martong added a comment.EditedMay 26 2022, 6:33 AM

Nice work, looks promising! Once the test file related comment is addressed I will accept.

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

You mean the enforced errno check could cause false positive?

clang/test/Analysis/errno-stdlibraryfunctions.c
21–25

+1 for @steakhal's recommendation.

balazske marked 5 inline comments as done.May 30 2022, 8:20 AM
balazske updated this revision to Diff 432930.May 30 2022, 8:22 AM

applied the review comments

martong accepted this revision.Jun 1 2022, 8:48 AM

LGTM!

This revision is now accepted and ready to land.Jun 1 2022, 8:48 AM
balazske updated this revision to Diff 435082.Jun 8 2022, 2:35 AM

small test change, removed a compile warning

martong added inline comments.Jun 9 2022, 3:08 AM
clang/test/Analysis/errno-stdlibraryfunctions-notes.c
11 ↗(On Diff #435082)

Do you plan to extend the test file for other standard functions as well?

clang/test/Analysis/errno-stdlibraryfunctions.c
15–16

Do you plan to extend the test file for other standard functions as well?

44–45

This should be reachable as well, isn't it?

balazske marked 2 inline comments as done.Jun 9 2022, 6:24 AM
balazske added inline comments.
clang/test/Analysis/errno-stdlibraryfunctions-notes.c
11 ↗(On Diff #435082)

It was not planned. I am not sure if it is better to have some more functions but not all. If there is a test for all of the functions it should be generated somehow (test every function for success and failure case, arguments passed to the functions are probably not easy to generate).

balazske updated this revision to Diff 435558.Jun 9 2022, 8:16 AM

small fix in test

martong accepted this revision.Jun 9 2022, 8:23 AM
steakhal requested changes to this revision.Jun 9 2022, 11:23 AM

It's getting in really good shape. Nice work.
I've got some nits about docs and checker dependencies and also a major comment inline.

The documentation needs to be updated to keep it in sync.
It should demonstrate the new warnings, state why we have them. Remember to refer back to the CERT rule for additional resources.
Refer to the related checker, in this case to ErrnoModeling, and explain how it changes the analysis.
Shouldn't it be a modeling dependency rather?

clang/test/Analysis/errno-stdlibraryfunctions-notes.c
15–23 ↗(On Diff #435558)

I must say that I dislike that one cannot disable this warning unless they disable the whole stdlibrarymodeling. It's getting bigger and bigger, thus increasing the value it provides. Yet, not everybody wants to follow SEI CERT guidelines - and I would expect plenty of users in that category, who might be surprised that reading errno is a bug.

Just imagine in the example that between the two access() calls, you set errno to zero.
The code would look reasonable.

  1. reset errno
  2. do some syscall
  3. check errno

I would expect this to be a good practice. We should at least have an option for suppressing reports for such scenarios. IMO it should be rather opt-in, than opt-out, but it might be my personal preference.
WDYT @martong @xazax.hun

This revision now requires changes to proceed.Jun 9 2022, 11:23 AM

The documentation of ErrnoChecker is added in the previous change D122150. Really this documentation should be updated when any checker is changed related to errno modeling (errno modeling can be added later to any checker that evaluates standard functions, for example StreamChecker). Is it required to mention the ErrnoModeling dependency? (The modeling checker is turned on always?) Or add documentation for ErrnoModeling too?

clang/test/Analysis/errno-stdlibraryfunctions-notes.c
15–23 ↗(On Diff #435558)

The CERT rule says that there is no guarantee that errno is not changed if the function is successful, regardless if it was 0 or not. There are some functions (that have in-band return value) where it is required to set errno to 0, there can be another checker (or extend ErrnoChecker) to check this.

The new warning is turned on when alpha.unix.Errno is turned on, otherwise it should not appear.

martong added inline comments.Jun 13 2022, 8:49 AM
clang/test/Analysis/errno-stdlibraryfunctions-notes.c
15–23 ↗(On Diff #435558)

I must say that I dislike that one cannot disable this warning unless they disable the whole stdlibrarymodeling.

Yeah, good point. Perhaps we should have a new checker option that enables/disables errno related reports. Should this be part of the ErrnoChecker (or the StdLibraryFunctionsChecker)?

It looks everything fine, given that the alpha.unix.Errno will depend on the apimodeling.StdCLibraryFunctions.
This should do the expected errno state propagation by which the Errno checker can report diagnostics - WHILE the StdCLibraryFunctions still won't report diagnostics.
This means that the Errno part /diagnostics can be separately disabled from the diagnostics of StdCLibraryFunctionArgsChecker and other frontend checkers of the StdCLibraryFunctionsChecker modeling part.
Excellent.

Please demonstrate that indeed disabling separately Errno or StdCLibraryFunctionArgsChecker will make the respective reports disappear.


I can see that in many cases you introduced an additional .Case(), which will result in a state-split.
I'm curious what runtime implications this change introduces. Please measure this.

I must also admit that I haven't checked the parameters of Case() in all cases; so I would encourage you to double-check those to make sure they are in line with the API specification.

Given these changes are implemented, I think it's ready to land. Awesome job, really!
Sorry about reject, I just saw it's green and I still had some thoughts about this.

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
68–69
446

nit: we should not use the NN and other suffixed versions of this API. The generic one should suffice.
Actually, I plan to refactor all code and make those private.

clang/test/Analysis/errno-stdlibraryfunctions-notes.c
15–23 ↗(On Diff #435558)

The new warning is turned on when alpha.unix.Errno is turned on, otherwise it should not appear.

Oh, I see. I might overreacted this part.

In the describeErrnoCheckState() the Errno_MustBeChecked is uncovered by tests.

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

I would prefer the west-const style.

balazske updated this revision to Diff 437924.Jun 17 2022, 8:37 AM
balazske marked 2 inline comments as done.

Small fixes.

steakhal accepted this revision.Jun 19 2022, 12:52 AM

LGTM, good job.

This revision is now accepted and ready to land.Jun 19 2022, 12:52 AM
balazske marked an inline comment as done.Jun 20 2022, 12:46 AM

I did performance measurements in the past and it showed no important difference, run time got more with at most some percent at some of the projects.

This revision was landed with ongoing or failed builds.Jun 20 2022, 11:57 PM
This revision was automatically updated to reflect the committed changes.