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.

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

Possibly better handling of MSAN reports #863

Closed
LebedevRI opened this issue Sep 27, 2017 · 13 comments
Closed

Possibly better handling of MSAN reports #863

LebedevRI opened this issue Sep 27, 2017 · 13 comments

Comments

@LebedevRI
Copy link
Contributor

I may be completely wrong here, but i'm under the impression that currently the reports
from memory sanitizer are deduplicated by the "Crash State", and then reported, correct?

Consider the following pseudo-code:

// both functions do some different stuff with memory
// but both of them do *not* init all of the memory
void f0(char* mem); 
void f1(char* mem);

int main(int argc, char* argv[]) {
  char buf[32];

  if(argc == 1)
   f0(&buf[0]);
  else
   f1(&buf[0]);

  __msan_check_mem_is_initialized(&buf[0], sizeof(buf)); // ta-da!
}

There are two distinct problems, but currently i believe oss-fuzz would only report one,
and only after it is fixed, report the second-one.

@Dor1s
Copy link
Contributor

Dor1s commented Sep 27, 2017

Why are there two distinct problems? It looks like buf is not initialized, that's it. What else?

@LebedevRI
Copy link
Contributor Author

This is just a dumb example to show the problem :)
The f0 / f1 are supposed to fully populate the buffer.

In the real code the buf is some image, which definitively should not be pre-initialized,
and the functions absolutely should correctly fully fill it.

This is a real problem i'm having (well, finding/fixing) in the rawspeed

@Dor1s
Copy link
Contributor

Dor1s commented Sep 27, 2017

But how do you want to deduplicate those cases if the use-of-uninitialized-value happens outside of f0 / f1?

@oliverchang
Copy link
Collaborator

I'm not sure if there's anything we can do here to catch this case as distinct reports -- it seems to me that this is not feasible.

I'm going to say this is WAI, but happy to discuss more if you have any suggestions though.

@LebedevRI
Copy link
Contributor Author

By comparing the sets of Features each of the crash testcase triggers.
But i agree that there are likely cases where it would erroneously detect the same problem as multiple.

@LebedevRI
Copy link
Contributor Author

Another idea. Please compare these two issues:
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=3531
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=3497

The Stacktraces specified in "Uninitialized value was created by a heap allocation" differ.
So perhaps accounting for that while deduplicating could work?

That wouldn't help with the case i presented in the first comment here, obviously.

@Dor1s
Copy link
Contributor

Dor1s commented Oct 6, 2017

Do you want these two bugs be filed as a single one? I think it would be so, but the second bug got fixed before the first got filed, this is why those are reporter separately. //cc @inferno-chromium

@LebedevRI
Copy link
Contributor Author

Do you want these two bugs be filed as a single one?

No, exactly the opposite.

the second bug got fixed before the first got filed

That is exactly the problem.

So far, there is exactly one of these bugs open at any point in time.
I fix the open issue, it is auto-closed, and the new issue is reported after that.
Thus i think they are being deduplicated, hiding different issues.

Thus i'm bringing up the issue, that deduplication is not ideal.

@LebedevRI
Copy link
Contributor Author

Any thoughts on this? Or would taking the allocation trace into account result in too many duplicates too?

@Dor1s
Copy link
Contributor

Dor1s commented Oct 11, 2017

That sounds like a potential improvement. Not a priority right now, but let's keep the issue open. Thanks!

@Dor1s Dor1s reopened this Oct 11, 2017
@LebedevRI
Copy link
Contributor Author

And the pattern continues...
I don't suppose the code that is used for that report deduplication is public and open-source, so i can provide a patch? :)

@Dor1s
Copy link
Contributor

Dor1s commented Oct 17, 2017

Yeah, the code is not open source...

@inferno-chromium
Copy link
Collaborator

@LebedevRI - code is open source and patch can be provided there if possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants