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

[PRISM] Fix crash when --parser=prism called with stdin #9282

Merged

Conversation

eightbitraptor
Copy link
Contributor

@eightbitraptor eightbitraptor commented Dec 18, 2023

Currently Ruby crashes when the --parser=prism flag is used either with no input, or with input that is being redirected from stdin. So all of the following will crash

ruby --parser=prism
ruby --parser=prism < test_code.rb
cat test_code.rb | ruby --parser=prism

This commit checks whether the input is assumed to be from stdin, and then processes that as a file.

This will fix the second and third case above, but will cause a slight behavioural changes for the first case - Ruby will treat stdin as an empty file in this case and exit, rather than waiting for data to be piped into stdin.

@HParker
Copy link
Contributor

HParker commented Dec 18, 2023

Thanks for finding this! The fix looks good to me. We could maybe add a test in test/ruby/test_rubyoptions.rb, but I am not sure if that is necessary.

Are you planning to open a PR to fix the first case as well? I can look, but I don't believe I have any special knowledge here beyond what you have in this PR.

Copy link
Member

@tenderlove tenderlove left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a good stopgap for now.

@tenderlove
Copy link
Member

This is related to this bug: ruby/prism#1247

@eightbitraptor
Copy link
Contributor Author

Thanks for finding this! The fix looks good to me. We could maybe add a test in test/ruby/test_rubyoptions.rb, but I am not sure if that is necessary.

Are you planning to open a PR to fix the first case as well? I can look, but I don't believe I have any special knowledge here beyond what you have in this PR.

Thanks! I'll take a look at adding a test to this and then get it shipped

As far as a proper fix - I haven't looked into it in major detail yet, but I think that Prism needs to handle this case rather than dealing with it in the options processing code. And I think this issue will need addressed first.

[Bug #20071]

Currently Ruby crashes when the --parser=prism flag is used either with
no input, or with input that is being redirected from stdin. So all of
the following will crash

ruby --parser=prism
ruby --parser=prism < test_code.rb
cat test_code.rb | ruby --parser=prism

This commit checks whether the input is assumed to be from stdin, and
then processes that as a file.

This will fix the second and third case above, but will cause a slight
behavioural changes for the first case - Ruby will treat stdin as an
empty file in this case and exit, rather than waiting for data to be
piped into stdin.
@eightbitraptor eightbitraptor merged commit 893fe30 into ruby:master Dec 18, 2023
97 checks passed
@eightbitraptor eightbitraptor deleted the mvh-fix-prism-cmdline-segv branch December 18, 2023 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants