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 edge cases in code parser and stringifier (found by fuzzing) #7886

Merged
merged 4 commits into from
Jun 14, 2019

Conversation

oprypin
Copy link
Member

@oprypin oprypin commented Jun 13, 2019

@oprypin oprypin changed the title Fix bugs found by fuzzing Fix parser bugs found by fuzzing Jun 13, 2019
@oprypin oprypin force-pushed the fuzz1 branch 2 times, most recently from 95d8253 to ee3b3d9 Compare June 13, 2019 22:53
@straight-shoota straight-shoota added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler:parser labels Jun 14, 2019
@oprypin
Copy link
Member Author

oprypin commented Jun 14, 2019

It's failing though... Wasn't able to figure out ranges+newlines yet

That is, very large integers *with* the suffix specified.
It was unconditionally converting the value to uint64, but have to check for the validity of that first.
Additional check for num_size < 20 fixes it. Such numbers definitely fit into uint64. And this condition fully covers int64, too. gen_check_uint_fits_in_size is not used for uint64 so this is fine.
Parsing/stringifying a macro body would always add two spaces and a newline, but it's supposed to stop changing after one pass.
@oprypin
Copy link
Member Author

oprypin commented Jun 14, 2019

Excluded the failing commit, will get back to that one later.

@oprypin
Copy link
Member Author

oprypin commented Jun 14, 2019

(in another PR; so this one is ready)

@RX14 RX14 requested a review from asterite June 14, 2019 13:55
@bcardiff
Copy link
Member

Thanks! Can the PR be named in a way it reflects the impact and no the procedure so it’s more meaningful as a changelog entry in the future?

@oprypin
Copy link
Member Author

oprypin commented Jun 14, 2019

The commits are meaningful changelog entries, the PR is indeed arbitrarily grouped by procedure.

@oprypin oprypin changed the title Fix parser bugs found by fuzzing Fix edge cases in code parser and stringifier (found by fuzzing) Jun 14, 2019
@asterite
Copy link
Member

I think Range and newline doesn't need to be supported because 1.. \n is parsed as an endless range now.

@oprypin
Copy link
Member Author

oprypin commented Jun 14, 2019

Alright, then I will not attempt to cover it. Seems consistent with the rest of the language, things like

(1
+2) == 2

But still doing it in a separate PR.
#7888

@asterite
Copy link
Member

Yes, now I'm actually not sure, probably endless range only makes sense when ) comes afterwards.

@oprypin
Copy link
Member Author

oprypin commented Jun 14, 2019

Well then, good that I split out the controversial thing.
This PR is still good though! :D

@straight-shoota straight-shoota merged commit 2595905 into crystal-lang:master Jun 14, 2019
@straight-shoota
Copy link
Member

Thank you @oprypin !

@straight-shoota straight-shoota added this to the 0.30.0 milestone Jun 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler:parser
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants