Bug #9040

Readline duplicate file descriptors but doesn't close them

Added by Eamonn Webster 6 months ago. Updated 6 months ago.

[ruby-core:57951]
Status:Closed
Priority:Normal
Assignee:-
Category:core
Target version:2.1.0
ruby -v:ruby 2.1.0dev (2013-09-22 trunk 43011) [x86_64-darwin12.5.0] Backport:1.9.3: UNKNOWN, 2.0.0: UNKNOWN

Description

This depends on the max open files limit, happens quicker the lower the limit.
irb crashes just by holding down return. Uses two file descriptors per prompt.

input=': Too many open files - dup (Errno::EMFILE)

or if you don't want to hold down the key...

ulimit -n 100
ruby -r readline -e "100.times{ Readline.input = STDIN }"

A recent patch to readline to avoid a segv when the underlying FILE has been closed, changed the way that the input and output streams
are assigned.

When a stream is assigned, its file descriptor is extracted, dup'ed and passed to fdopen.
As the file descriptor is dup'ed the two file descriptors (in the FILE owned by the readline library and the one inside the ruby rbiot)
don't match.

Before assigning the previous value should be cleared. But this only happens when the ruby stream has been closed or when the two file descriptors are the same (never).

As we always dup the file descriptors, we own them, and should always close them.

readline_fix.patch Magnifier (2.51 KB) Eamonn Webster, 10/21/2013 09:36 PM

readline-release-gvl-3.patch Magnifier (8.06 KB) Akira Tanaka, 10/21/2013 11:51 PM

readline-release-gvl-4.patch Magnifier (7.44 KB) Akira Tanaka, 10/22/2013 07:47 PM

readline-release-gvl-5.patch Magnifier (7.99 KB) Akira Tanaka, 10/23/2013 06:17 PM

Associated revisions

Revision 43439
Added by Akira Tanaka 6 months ago

  • ext/readline/readline.c: Include ruby/thread.h for
    rbthreadcallwithoutgvl2.
    (readlinerlinstream, readlinerloutstream): Record FILE
    structures allocated by this extension.
    (getcbody): New function extracted from readlinegetc.
    (getcfunc): New function.
    (readline
    getc): Use rbthreadcallwithoutgvl2 to invoke getcfunc.
    [Bug #8749]
    (clear
    rlinstream, clearrloutstream): Close FILE structure
    allocated by this extention reliably. [Bug #9040]
    (readline
    readline): Use clearrlinstream and clearrloutstream.
    (readlinessetinput): Set readlinerlinstream.
    (readline
    ssetoutput): Set readlinerloutstream.
    (Initreadline): Don't call readlinessetinput because
    readline_getc doesn't block other threads for any FILE structure now.

    [Bug #8749] reported by Nobuhiro IMAI.
    [Bug #9040] reporeted by Eamonn Webster.

History

#1 Updated by Akira Tanaka 6 months ago

I think it's better to not trust the value of rlinstream and rloutstream
because they can be modified by other libraries.

I updated my readline patch submitted to [Bug #8749].

#2 Updated by Eric Wong 6 months ago

"akr (Akira Tanaka)" akr@fsij.org wrote:

Issue #9040 has been updated by akr (Akira Tanaka).

File readline-release-gvl-3.patch added

Does poll work reliably with tty FD on non-Linux systems?

Perhaps better to use rbthreadcallwithgvl and a
rbwaitforsinglefd wrapper

#3 Updated by Akira Tanaka 6 months ago

normalperson (Eric Wong) wrote:

Does poll work reliably with tty FD on non-Linux systems?

Perhaps better to use rbthreadcallwithgvl and a
rbwaitforsinglefd wrapper

I see. I updated the patch.

#4 Updated by Eric Wong 6 months ago

"akr (Akira Tanaka)" akr@fsij.org wrote:

Issue #9040 has been updated by akr (Akira Tanaka).

File readline-release-gvl-4.patch added

Fewer ifdefs, so good :> (haven't tested)

Btw, on a separate note, it would be a good idea to check the return
value of fileno() in case another extension accidentally fclose()

#5 Updated by Akira Tanaka 6 months ago

normalperson (Eric Wong) wrote:

Btw, on a separate note, it would be a good idea to check the return
value of fileno() in case another extension accidentally fclose()

I think avoiding accidental fclose() is not the responsibility of readlinegetc().
It is because readline
getc() cannot determine the FILE structure is closed or not.
So the only appropriate action for the situation is rb_bug()
because fileno() for a closed stream is undefined behavior
(possibly SEGV if memory for FILE structure is returned to OS).

I updated the patch.

#6 Updated by Eric Wong 6 months ago

"akr (Akira Tanaka)" akr@fsij.org wrote:

Issue #9040 has been updated by akr (Akira Tanaka).

File readline-release-gvl-5.patch added

normalperson (Eric Wong) wrote:

Btw, on a separate note, it would be a good idea to check the return
value of fileno() in case another extension accidentally fclose()

I think avoiding accidental fclose() is not the responsibility of readlinegetc().
It is because readline
getc() cannot determine the FILE structure is closed or not.
So the only appropriate action for the situation is rb_bug()
because fileno() for a closed stream is undefined behavior
(possibly SEGV if memory for FILE structure is returned to OS).

Agreed, thanks.

On a related note: should rbfdset/rbfdresize call rb_bug on
negative FD? Otherwise it could OOM/SEGV, I think

#7 Updated by Akira Tanaka 6 months ago

2013/10/24 Eric Wong normalperson@yhbt.net:

On a related note: should rbfdset/rbfdresize call rb_bug on
negative FD? Otherwise it could OOM/SEGV, I think

It's possible to fail fast.

I'm not sure wihch is suitable between rbbug and rbraise, though.
Negative FD can be determined safely.
--
Tanaka Akira

#8 Updated by Akira Tanaka 6 months ago

  • Status changed from Open to Closed
  • % Done changed from 0 to 100

This issue was solved with changeset r43439.
Eamonn, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.


  • ext/readline/readline.c: Include ruby/thread.h for
    rbthreadcallwithoutgvl2.
    (readlinerlinstream, readlinerloutstream): Record FILE
    structures allocated by this extension.
    (getcbody): New function extracted from readlinegetc.
    (getcfunc): New function.
    (readline
    getc): Use rbthreadcallwithoutgvl2 to invoke getcfunc.
    [Bug #8749]
    (clear
    rlinstream, clearrloutstream): Close FILE structure
    allocated by this extention reliably. [Bug #9040]
    (readline
    readline): Use clearrlinstream and clearrloutstream.
    (readlinessetinput): Set readlinerlinstream.
    (readline
    ssetoutput): Set readlinerloutstream.
    (Initreadline): Don't call readlinessetinput because
    readline_getc doesn't block other threads for any FILE structure now.

    [Bug #8749] reported by Nobuhiro IMAI.
    [Bug #9040] reporeted by Eamonn Webster.

Also available in: Atom PDF