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

Fix some problems turned up by ossfuzz #4927

Merged
merged 7 commits into from
Jan 22, 2022

Conversation

greglandrum
Copy link
Member

Looking at the results of some of the more serious fuzz test failures led to this series of fixes.

Some of these are for undefined behavior (mainly due to static_casting the wrong way in the type hierarchy), but some are general defensive programming when it comes to parsing inputs. Most of the defensive programming stuff won't matter with real inputs, but it doesn't hurt to tighten up the parsers some.

@greglandrum greglandrum added this to the 2021_09_5 milestone Jan 20, 2022
@bp-kelley
Copy link
Contributor

LGTM, I like the dynamic_cast here, I was always a bit queasy about the static casts.

Some of these look like API changes, does there need to be a change list for these?

@greglandrum
Copy link
Member Author

Some of these look like API changes, does there need to be a change list for these?

None of the changes should break existing code. We lose ABI compatibility (so a recompile will be required), but that's not something I even try to maintain.

It is a good point that with changes to the API like this it probably shouldn't be part of a patch release. I'll change the milestone.

@greglandrum greglandrum modified the milestones: 2021_09_5, 2022_03_1 Jan 22, 2022
@greglandrum greglandrum merged commit db15b37 into rdkit:master Jan 22, 2022
@greglandrum greglandrum deleted the fix/more_ossfuzz_errors branch January 22, 2022 03:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants