Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[analyzer] getenv() state split missing a note. #53276

Closed
haoNoQ opened this issue Jan 18, 2022 · 7 comments
Closed

[analyzer] getenv() state split missing a note. #53276

haoNoQ opened this issue Jan 18, 2022 · 7 comments
Assignees
@steakhal
Labels
clang:static analyzer

Comments

@haoNoQ
Copy link
Collaborator

haoNoQ commented Jan 18, 2022

Now that we're bifurcating on getenv() by default, we should explain to the user why we think that the pointer may be null. Eg. this isn't an understandable report:

Screen Shot 2022-01-18 at 2 51 43 PM

Ideally this should be a two-step report with the first step saying something like "Assuming the variable is not present in the environment".

@martong @steakhal how close are we to attaching note tag material to summaries?

P.S. I love the new bugzilla, was just waiting for a chance to use it^^
@haoNoQ haoNoQ changed the title getenv() state split missing a note. [analyzer] getenv() state split missing a note. Jan 18, 2022
@steakhal
Copy link
Contributor

Well, AFAIK there is currently no way in the StdLibraryChecker to add a NoteTag for a Case(). It shouldn't be hard to extend it with a "message". Or even better, with the NoteTag lambda itself. That way we could implement custom interestingness conditions for the message to appear.
Of course, we could still provide a handy factory for creating the simple lambda wrapping the single string message for the generic case.
WDYT?

Even though the DSL is already on the complex side, these NoteTags would be beneficial in all cases. More often than not we are interested in why something is assumed by the analyzer. So, an addTransition() without a NoteTag is like a code smell for me.

@martong @steakhal how close are we to attaching note tag material to summaries?

That being said, we haven't thought about this, but we definitely should have.

@haoNoQ
Copy link
Collaborator Author

haoNoQ commented Mar 16, 2022 •
edited

Folks, are you actively working on this issue these days? I'm thinking of disabling the getenv split by default (eg., by disabling the controlled environment flag by default) if there's no quick fix for this issue.

@steakhal
Copy link
Contributor

I'm not working on it. Would you please reflect on the proposed implementation in my previous post?
If it looks good, I could pose this internally and maybe schedule it at some point.
Otherwise, disable it if you feel like it.

@haoNoQ
Copy link
Collaborator Author

haoNoQ commented Mar 16, 2022

Yes, I think a simple string message is probably sufficient. Especially given that a summary should almost never have more than two branches anyway (very hard to justify a more-than-2-way state split).

@haoNoQ
Copy link
Collaborator Author

haoNoQ commented Mar 16, 2022

So, an addTransition() without a NoteTag is like a code smell for me.

I agree. It's often conditional or prunable but most of the time it has to be there.

balazske added a commit to balazske/llvm-project that referenced this issue Apr 17, 2022
@balazske
[analyzer] Add path note tags to standard library function summaries.
c7d9b60 
This is a solution to the issue with `getenv()` (llvm#53276) but I covered a few more functions just because I could.

The patch is straightforward except the tiny fix in `BugReporterVisitors.cpp` that suppresses a default note for "Assuming pointer value is null" when a note tag from the checker is present. This is probably the right thing to do but also definitely not a complete solution to the problem of different sources of path notes being unaware of each other, which is a large and annoying issue that we have to deal with. Note tags really help there because they're nicely introspectable. The problem is demonstrated by the newly added `getenv()` test; I did not investigate why doesn't the original buggy report have the same note but I agree that this might be interesting to figure out.

The notes are currently optional but I think we should eventually implement all of them and then make them mandatory.

The notes are prunable, i.e. they won't bring-in entire stack frames worth of notes just because they're there, but they will be always visible regardless of whether the value is of interest to the bug report. I think this is debatable, the arguably better solution is to make them non-prunable but conditional to the value being tracked back to the call, which would probably need a better tracking infrastructure.

Differential Revision: https://reviews.llvm.org/D122285
@martong
Copy link
Collaborator

martong commented May 27, 2022

Solved by the referenced commit.

@martong martong closed this as completed May 27, 2022
@steakhal
Copy link
Contributor

Indeed, fixed by f68c0a2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@steakhal steakhal

Labels
clang:static analyzer
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@haoNoQ @martong @steakhal