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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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. | |
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. |
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.
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. |
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 |
update
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp | ||
---|---|---|
454–460 | It should work, FunctionName.str() and getOpcodeStr and describeErrnoCheckState should return non-temporary strings. |
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. |
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? |
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). |
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.
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. |
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. |
clang/test/Analysis/errno-stdlibraryfunctions-notes.c | ||
---|---|---|
15–23 ↗ | (On Diff #435558) |
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. | |
clang/test/Analysis/errno-stdlibraryfunctions-notes.c | ||
15–23 ↗ | (On Diff #435558) |
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. |
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.