-
Notifications
You must be signed in to change notification settings - Fork 845
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
Fix vulnerabilities found by fuzzer. #6579
Conversation
@thomp-j thanks for the contribution. |
This is an attempt to double check where the test failure is coming from.
Re-adding changes to test tests.
Thanks for re-running. I was hoping that was the case - things look better now. |
Yep. If you are ready I will go ahead and do the "review" and merge the fix |
I think we are ready to go! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@thomp-j thanks for the contribution! |
* Fix vulnerabilities found by fuzzer. * Fix Typo in RWMol.cpp * Update RWMol.cpp This is an attempt to double check where the test failure is coming from. * Update RWMol.cpp Re-adding changes to test tests.
Reference Issue
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=49783
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=44520
What does this implement/fix? Explain your changes.
(1) During fuzzing, it's possible that one atom object is added to the same Stereo group multiple times. The replaceAtom function only replaces a single occurrence. The code later references the second copy of the atom in the stereo group (which wasn't replaced) which accesses deleted memory. This change replaces all copies of an atom during the replaceAtom process.
(2) Catches situations during fuzzing where index values for text substrings in ParseOldAtomList could go out of range.
Any other comments?