This is an archive of the discontinued LLVM Phabricator instance.

[clang][analyzer] Add some more functions to StreamChecker and StdLibraryFunctionsChecker.
AbandonedPublic

Authored by balazske on Oct 6 2022, 6:18 AM.

Details

Summary

Add support for functions 'fgetpos', 'fsetpos', 'ftell', 'rewind'.

Diff Detail

Event Timeline

balazske created this revision.Oct 6 2022, 6:18 AM
Herald added a reviewer: NoQ. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
balazske requested review of this revision.Oct 6 2022, 6:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 6 2022, 6:18 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

I found some anomalies during development:

  • If the checker StdCLibraryFunctions is added as dependency for alpha.unix.Stream in checkers.td I get some "unexplainable" test failures.
  • In the comments section at CERT ERR30-C is pointed out that function ftell can change the value of errno if it is successful. This is the "normal" expected behavior of all standard functions unless the documentation tells other. But this place (search for ftell) tells that ftell should not change errno if successful. The persons commented at the CERT rule probably checked only one or more of the C standards. But there should be no conflict between POSIX and C ("Any conflict between the requirements described here and the ISO C standard is unintentional."). It would be unconventional if different check rules are needed (and add of a global analyzer option for POSIX or C mode).

I found some anomalies during development:

  • If the checker StdCLibraryFunctions is added as dependency for alpha.unix.Stream in checkers.td I get some "unexplainable" test failures.

Could you please elaborate? I don't see how to help you with it without seeing more details.

  • In the comments section at CERT ERR30-C is pointed out that function ftell can change the value of errno if it is successful. This is the "normal" expected behavior of all standard functions unless the documentation tells other. But this place (search for ftell) tells that ftell should not change errno if successful. The persons commented at the CERT rule probably checked only one or more of the C standards. But there should be no conflict between POSIX and C ("Any conflict between the requirements described here and the ISO C standard is unintentional."). It would be unconventional if different check rules are needed (and add of a global analyzer option for POSIX or C mode).

When it is ambiguous then I'd check the latest standards of both POSIX and C. If it is still doubtful then I'd vote for the C standard and would report a defect towards the POSIX community.

martong added inline comments.Oct 17 2022, 7:32 AM
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
1847–1886

It is very good to see these new summaries! Would they be meaningful and usable without the changes in the StreamChecker? If yes, then I think we should split this into 2 patches.

If we look only at the C standard we can not tell much about if the functions should set errno. It seems that setting errno is totally implementation-dependent, except for a few functions that mention errno. These are fgetpos, fsetpos, ftell and should set errno to a positive value on error (but there is no guarantee that value of errno is preserved if no error occurs). And errno is always positive. This is different than what the checker currently does (with the new patches). Probably this should be a discourse question?

If we look only at the C standard we can not tell much about if the functions should set errno. It seems that setting errno is totally implementation-dependent, except for a few functions that mention errno. These are fgetpos, fsetpos, ftell and should set errno to a positive value on error (but there is no guarantee that value of errno is preserved if no error occurs). And errno is always positive. This is different than what the checker currently does (with the new patches). Probably this should be a discourse question?

Okay then, I think it is worth to have a discourse question. But you could ask the wider "Clang" community, so I would not post the question as something that is related strictly to the static analyzer.

About the "ftell" problem: The POSIX rules are really an extension of the C standard rules. At ftell according to C standard errno should be set to a positive value if error occurs. The POSIX rules extend this: errno is not changed if no error occurs. This can be correct, then the CERT ftell non-compliant example is wrong (on success errno remains always 0). It can be fixed if line errno=0 is removed from the start of the non-compliant code. It may be best to use the POSIX rules for the checker, because the C standard does not say much and may need to require setting of errno to 0 before a standard function call.

balazske updated this revision to Diff 473659.Nov 7 2022, 7:19 AM

Added more functions to StdLibraryFunctionsChecker.
This is redundant with the StreamChecker for now but
makes possible to remove the NULL stream check from
StreamChecker and reduce overlap of the checkers.
This way no dependency should be needed between these checkers.

balazske updated this revision to Diff 474558.Nov 10 2022, 8:39 AM

Rebase to main and D137722.

I found some anomalies during development:

  • If the checker StdCLibraryFunctions is added as dependency for alpha.unix.Stream in checkers.td I get some "unexplainable" test failures.

Could you please elaborate? I don't see how to help you with it without seeing more details.

Mind that dependencies also establish the order of callbacks (dependent checkers are called after their dependencies).

When it is ambiguous then I'd check the latest standards of both POSIX and C. If it is still doubtful then I'd vote for the C standard and would report a defect towards the POSIX community.

About the "ftell" problem: The POSIX rules are really an extension of the C standard rules. At ftell according to C standard errno should be set to a positive value if error occurs. The POSIX rules extend this: errno is not changed if no error occurs. [...] It may be best to use the POSIX rules for the checker, because the C standard does not say much and may need to require setting of errno to 0 before a standard function call.

Interesting pair of perspectives, I think a reasonable checker should be a little more conservative, more akin to what POSIX seems to specify.

[...] Probably this should be a discourse question?

Okay then, I think it is worth to have a discourse question. But you could ask the wider "Clang" community, so I would not post the question as something that is related strictly to the static analyzer.

Did this thread ever materialize? I admit I didn't follow :)

My current approach is that the POSIX is more strict than the C standard (POSIX allows a subset of what C allows). I do not see (errno related) contradiction between these standards (except initialization to zero of errno missing from POSIX, and possible negative value in errno in POSIX). The checker currently works only in POSIX mode for every function, the ModelPOSIX setting controls only if additional functions are added (all with POSIX rules) (these functions could be added with C rules too). One problem exists now, that errno is set to a non-zero (but not positive only) value at error. Probably we should change this to be always positive at error, according to the C standard errno is positive only, and POSIX does not tell that errno can be negative.

Szelethus added a comment.EditedDec 13 2022, 4:12 AM

My current approach is that the POSIX is more strict than the C standard (POSIX allows a subset of what C allows). I do not see (errno related) contradiction between these standards

Alright, so if we wanna support the C standard thinking, we would just create an option that would be a little more noisy. Sounds fair enough.

(except initialization to zero of errno missing from POSIX, and possible negative value in errno in POSIX). One problem exists now, that errno is set to a non-zero (but not positive only) value at error. Probably we should change this to be always positive at error, according to the C standard errno is positive only, and POSIX does not tell that errno can be negative.

According to this (which I assume you've seen as well): https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/errno.h.html

Description

[...]
Some of the functionality described on this reference page extends the ISO C standard. Any conflict between the requirements described here and the ISO C standard is unintentional. [...]
[...]

Issue 6

Values for errno are now required to be distinct positive values rather than non-zero values. This change is for alignment with the ISO/IEC 9899:1999 standard.

So yes, I'd say its reasonable to assume in POSIX modthat if errno is non-zero, it is positive.

The checker currently works only in POSIX mode for every function, the ModelPOSIX setting controls only if additional functions are added (all with POSIX rules) (these functions could be added with C rules too).

I see a lot of mention of POSIX vs. the C standard, yet there seems to be no mention of these differences in the comments added by this patch or already commited. Maybe we could do better on documenting this important difference in behaviour.

Okay then, I think it is worth to have a discourse question. But you could ask the wider "Clang" community, so I would not post the question as something that is related strictly to the static analyzer.

Did this thread ever materialize? I admit I didn't follow :)

I suppose the answer is no then :)

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
1847–1886

Can you please do this?

balazske added inline comments.Dec 20 2022, 8:59 AM
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
1847–1886

This is done now. There were 2 consecutive patches that modify StdLibraryFunctionsChecker and other checkers. These are rearranged into 2 new patches. One for all changes in StdLibraryFunctionsChecker (and related changes in ErrnoModeling): D140387. Other for all other changes, StreamChecker and many of the tests that exercise both checkers: D140395.