Skip to content

Fix an infinite loop when attempting to read from a directory#4817

Closed
philr wants to merge 1 commit intojruby:masterfrom
philr:fix_infinite_loop_reading_directory
Closed

Fix an infinite loop when attempting to read from a directory#4817
philr wants to merge 1 commit intojruby:masterfrom
philr:fix_infinite_loop_reading_directory

Conversation

@philr
Copy link
Contributor

@philr philr commented Oct 15, 2017 •
edited
Loading

When attempting to open a directory with File.open and then read from it, JRuby 9.1.13.0 (on platforms other than Windows) will enter an infinite loop and hang:

> Timeout::timeout(10) { File.open('.') {|f| f.read } }
Timeout::Error: execution expired
        from org/jruby/RubyIO.java:3009:in `read'
        from org/jruby/RubyIO.java:2987:in `read'
        from (irb):1:in `block in evaluate'
        from org/jruby/RubyIO.java:1156:in `open'
        from (irb):1:in `block in evaluate'
        from org/jruby/ext/timeout/Timeout.java:117:in `timeout'
        from (irb):1:in `<eval>'

When attempting to do the same with MRI (tested with versions 1.9.3 through to 2.4.2) an Errno::EISDIR exception is raised:

> File.open('.') {|f| f.read }
Errno::EISDIR: Is a directory @ io_fread - .
        from (irb):1:in `read'
        from (irb):1:in `block in irb_binding'
        from (irb):1:in `open'
        from (irb):1

The hang occurs because OpenFile.fillbuf() and OpenFile.ioBufread() loop to retry on errors. This is desirable for temporary errors like EAGAIN, but not for permanent errors like EISDIR.

This pull request modifies OpenFile.waitReadable() to abort retries when a permanent error is encountered. It also changes Helpers.errnoFromException() to map the "Is a directory" IOException message to EISDIR.

This change only applies to non-Windows platforms. On Windows, File.open raises an exception instead.

@philr
Fix an infinite loop when attempting to read from a directory.
f671a94 
Raise Errno::EISDIR instead to match MRI.
@kares
Copy link
Member

kares commented Oct 23, 2017

have tried running this against jruby-9.1 but this time there seems to be more failures from test:jruby
... than before (last jruby-9.1 build here) : https://travis-ci.org/kares/jruby/jobs/291393645

@philr
Copy link
Contributor Author

philr commented Oct 28, 2017

@kares I'm seeing the same test failures from test:jruby in the build of the merge of this change to your jruby-9.1 branch (https://travis-ci.org/kares/jruby/jobs/291393645) as in the build of the parent commit (https://travis-ci.org/jruby/jruby/jobs/289840028). The following tests fail in both builds:

  • test_ipv6_loopback?
  • test_directory_query
  • test_executable?_query
  • test_executable_real?_query
  • test_file_exist_query
  • test_file_query
  • test_readable_query

I've just merged this onto jruby-9.1 myself (philr/jruby@5d25f7b). Running test:jruby in my dev environment, I only see the test_ipv6_loopback? failure. This is the same outcome as the latest jruby-9.1 build (https://travis-ci.org/jruby/jruby/jobs/293860475).

@kares
Copy link
Member

kares commented Oct 29, 2017

oh really? have assumed they're builds from the same base, although I did not look carefully ...
hard to work with CI failures 😭 - but since jruby-9.1 only has the one let's have it for 9.1.14.0

@kares
Copy link
Member

kares commented Oct 29, 2017

done at 963e2a1 - thanks for the fix

@kares kares closed this Oct 29, 2017
@kares kares added this to the JRuby 9.1.14.0 milestone Oct 29, 2017
@philr philr deleted the fix_infinite_loop_reading_directory branch October 29, 2017 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

JRuby 9.1.14.0

Development

Successfully merging this pull request may close these issues.

2 participants

@philr @kares