Fix a bunch of crashes and leaks#32
Open
GabrielRavier wants to merge 22 commits intoClownacy:masterfrom
Open
Conversation
The crashes in question: - Dividing by zero through / or % would crash - Too-large shifts would crash under UBSAN (they're undefined behaviour) - Redefining a macro argument in a macro would crash - Using shift in a macro that already had 0 arguments would crash - Having a line with a comment followed by a line with an error would crash Also fixed a leak of the input file path.
In particular: - Source lines lists freeing wasn't actually freeing the source line - Symbol dictionaries only freed the nodes and not their contents (e.g. string constants or macros) - Macro invocations weren't freeing the "unique suffix" - Macro argument lists only had their first argument freed (repeatedly in a loop, but at least string destruction is such that it wouldn't crash)
Specifically: - Macro parameter names were not being freed - ResolveExpression had an almost complete copy-paste of DestroyExpression, but it was incomplete so wasn't freeing everything (replaced with just calling DestroyExpression direcftly) - PurgeMacro didn't destroy any of the macro's internal structures, so that work has been separated into a DestroyMacro function it now calls
Also fixed typo in error message ("POPO" -> "POPP")
This refactors DestroyMacro into a more generic DestroySymbol.
This also fixes a bug where the freeing of the rest of the shared macro state was being done in the wrong place
Just don't allow a symbol's type to be changed like this arbitrarily - it's of little value since there's already an error and my guess is there's other edge-cases than the one presented in the test case that would also trigger this.
This simply stops the redefinition - I suppose I could imagine scenarios where redefining a macro while it is being executed could be useful, but for now this seems sufficient
Enforces a fixed 2000 expansion depth limit. Also added a few asserts to AssembleLine.
This handles these cases far more generically than the previous patch that only worked for cases of multiple repts - this should hopefully correctly handle any combination of multiple while/rept/macro invocations being left unterminated
…sion This also refactors regression tests in CMakeLists.txt, in particular: - This makes them use a single central function to make things quite a bit simpler - This adds clownassembler_hash tests with proper hashes for those few tests that don't trigger any errors
…ut way If the following occurs: - code invokes macro a - macro a invokes macro b - macro b redefines macro a then the current checks would not detect that we're redefining a macro that's currently being expanded, since we only check for the "latest" current expanding macro. I suppose it might be possible to define macros while expanding another one, but it seems at least somewhat hard to support without this kind of breakage for now
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I found these through use of libFuzzer, Honggfuzz and AFL++ (if you want to see some of the gory details of that, or perhaps even do some more fuzzing yourself, I've pushed the setup I had for that on the
fuzzing-sandboxbranch on my repository).I hope the commit history isn't too messy - if you want, I can split these into ~25 different PRs, or even squash it all into a single commit, but as-is it seems at least mostly fine to me.