Skip to content

Fix Miscellaneous Regressions and UBs#1022

Merged
Headline merged 9 commits intomasterfrom
ub-fixes
May 29, 2019
Merged

Fix Miscellaneous Regressions and UBs#1022
Headline merged 9 commits intomasterfrom
ub-fixes

Conversation

@Headline
Copy link
Member

Ran the cppcheck static analyzer against /core/ to check if we were doing any ugliness that I didn't know about. Turns out there's a few situations where we were regressed.

First, and likely the most significant, was CExtension::FindNextDependency. This method must not have worked correctly for many many years. All operations were mistakenly done on a default-constructed iterator.

Secondly, realloc can fail. By doing assignment directly on the result of realloc we miss the opportunity to check if the retval is null or not. If it is null, then our memory allocated previously is still valid and we've just successfully leaked it. For the case involving InternalFormat, an exception was added for realloc failures.

Finally, 1<<31 is undefined behavior as 1 is an int32.

Also switched some casts to static_cast because this is c++, after all...

@Headline
Copy link
Member Author

Since dependency iteration has never historically functioned correctly, it is now deprecated.

Extensions have a very specific place alongside SourceMod, and their goal is to extend SourceMod's plugin API and provide an extension to SourceMod functionality. There was never really a need for this feature in the first place and this change allows us to further solidify our expectations for what an extension is meant to be.

@KyleSanderson KyleSanderson requested a review from dvander May 28, 2019 06:00
@Headline Headline added the Bug general bugs; can be anything label May 28, 2019
@Headline Headline merged commit 2803696 into master May 29, 2019
@Headline Headline deleted the ub-fixes branch May 29, 2019 01:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug general bugs; can be anything

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants