Bug #9040: Readline duplicate file descriptors but doesn't close them

• Status: Closed
• Assignee: -
• 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

[eweb (Eamonn Webster)]

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 rb_io_t)
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.

[akr (Akira Tanaka)]

• File readline-release-gvl-3.patch readline-release-gvl-3.patch added

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

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

[normalperson (Eric Wong)]

"akr (Akira Tanaka)" [e-mail] 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 rb_thread_call_with_gvl and a
rb_wait_for_single_fd wrapper

[akr (Akira Tanaka)]

• File readline-release-gvl-4.patch readline-release-gvl-4.patch added

normalperson (Eric Wong) wrote:

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

Perhaps better to use rb_thread_call_with_gvl and a
rb_wait_for_single_fd wrapper

I see. I updated the patch.

[normalperson (Eric Wong)]

"akr (Akira Tanaka)" [e-mail] 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()

[akr (Akira Tanaka)]

• File readline-release-gvl-5.patch 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 readline_getc().
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.

[normalperson (Eric Wong)]

"akr (Akira Tanaka)" [e-mail] 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 readline_getc().
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 rb_fd_set/rb_fd_resize call rb_bug on
negative FD? Otherwise it could OOM/SEGV, I think

[akr (Akira Tanaka)]

2013/10/24 Eric Wong [e-mail]:

On a related note: should rb_fd_set/rb_fd_resize 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 rb_bug and rb_raise, though.
Negative FD can be determined safely.

Tanaka Akira

[akr (Akira Tanaka)]

• 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
  rb_thread_call_without_gvl2.
  (readline_rl_instream, readline_rl_outstream): Record FILE
  structures allocated by this extension.
  (getc_body): New function extracted from readline_getc.
  (getc_func): New function.
  (readline_getc): Use rb_thread_call_without_gvl2 to invoke getc_func.
  [ruby-dev:47033] [Bug #8749]
  (clear_rl_instream, clear_rl_outstream): Close FILE structure
  allocated by this extention reliably. [ruby-core:57951] [Bug #9040]
  (readline_readline): Use clear_rl_instream and clear_rl_outstream.
  (readline_s_set_input): Set readline_rl_instream.
  (readline_s_set_output): Set readline_rl_outstream.
  (Init_readline): Don't call readline_s_set_input because
  readline_getc doesn't block other threads for any FILE structure now.

  [ruby-dev:47033] [Bug #8749] reported by Nobuhiro IMAI.
  [ruby-core:57951] [Bug #9040] reporeted by Eamonn Webster.

[wanabe (_ wanabe)]

• Related to Bug #12950: irb: 'input-method.rb:151: [BUG] Segmentation fault' / 'malloc(): smallbin double linked list corrupted' added