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

Check the file descriptor with IO#initialize; resolve #4966 #4968

Merged
merged 1 commit into from
Apr 15, 2020

Conversation

dearblue
Copy link
Contributor

No description provided.

@matz
Copy link
Member

matz commented Apr 14, 2020

The issue was fixed by 7add524. If there's any uncovered issue, tell me.

@matz matz closed this Apr 14, 2020
@dearblue
Copy link
Contributor Author

I don't think there is a problem with select.
# I wasn't aware of the FD_SETSIZE (I'm embarrassed).

However, it' s worthwhile to check file descriptors with IO#initialize:

  • CRuby removes bad file descriptors with IO#initialize.
    https://github.com/ruby/ruby/blob/v2_7_1/io.c#L8270-L8275

  • On Windows, if you give an invalid file descriptor to _get_osfhandle(), it seems to crash.
    This will cause a crash if you give it to a POSIX compatible file function (such as fstat(), read() and write()).

    It couldn't be replicated in wine, but I'm guessing from the failure in appveyor.
    This is cluttered with commit references in Segmentation fault at mrb_io_s_select #4966 by CI failures (I'm embarrassed this too), most of which occurred when I give _get_osfhandle() a non-existent file descriptor.

@matz
Copy link
Member

matz commented Apr 15, 2020

Ah, according to POSIX, all those system calls should fail gracefully with EBADF. I didn't know _get_osfhandle() crashes on Windows (sigh).

OK, I will merge this PR with a slight modification.

@matz matz reopened this Apr 15, 2020
@matz matz merged commit c91c9e2 into mruby:master Apr 15, 2020
@dearblue dearblue deleted the check-fd branch April 15, 2020 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants