Skip to content

Commit

Permalink
Reopen standard handles when they are a TTY (#6518)
Browse files Browse the repository at this point in the history
* Reopen standard handles when they are a TTY

Reopening the TTY prevents crystal from breaking other programs that are
attached to the same TTY when it sets O_NONBLOCK on its std filehandles.

Due to this change, when exec'ing a new process crystal no longer
unexpectedly sets its own std handles back to 'blocking'.

* Include the std_fd where needed

* Actually add the stdfilehandle file...

* Initialize the static array

* Don't set std handles to nonblocking after fork

Even setting NONBLOCK for a split second is dangerous.
From what I can see the child's stdin/out/err is supposed to be
blocking at all times, so just reopen the original FD's in
blocking mode.

There is still a problem with handling closed filehandles that needs
to be solved. `crystal_program | fast_exiting_child` crashes.. but this
is apparently not a regression.

* Move tty handler logic back to IO::FD

* Undocument `IO::FD.from_stdio` method

* Remove unneeded filehandle openness check
  • Loading branch information
Timbus authored and RX14 committed Aug 20, 2018
1 parent 202e5ac commit 2530bb6
Show file tree
Hide file tree
Showing 15 changed files with 36 additions and 40 deletions.
27 changes: 0 additions & 27 deletions src/crystal/main.cr
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,6 @@ lib LibCrystalMain
end

module Crystal
@@stdin_is_blocking = false
@@stdout_is_blocking = false
@@stderr_is_blocking = false

# Defines the main routine run by normal Crystal programs:
#
# - Initializes the GC
Expand Down Expand Up @@ -38,8 +34,6 @@ module Crystal
def self.main(&block)
GC.init

remember_blocking_state

status =
begin
yield
Expand All @@ -53,7 +47,6 @@ module Crystal
status = AtExitHandlers.run status
STDOUT.flush
STDERR.flush
restore_blocking_state

status
end
Expand Down Expand Up @@ -103,26 +96,6 @@ module Crystal
def self.main_user_code(argc : Int32, argv : UInt8**)
LibCrystalMain.__crystal_main(argc, argv)
end

# :nodoc:
def self.remember_blocking_state
{% if flag?(:win32) %}
@@stdin_is_blocking = true
@@stdout_is_blocking = true
@@stderr_is_blocking = true
{% else %}
@@stdin_is_blocking = IO::FileDescriptor.fcntl(0, LibC::F_GETFL) & LibC::O_NONBLOCK == 0
@@stdout_is_blocking = IO::FileDescriptor.fcntl(1, LibC::F_GETFL) & LibC::O_NONBLOCK == 0
@@stderr_is_blocking = IO::FileDescriptor.fcntl(2, LibC::F_GETFL) & LibC::O_NONBLOCK == 0
{% end %}
end

# :nodoc:
def self.restore_blocking_state
STDIN.blocking = @@stdin_is_blocking
STDOUT.blocking = @@stdout_is_blocking
STDERR.blocking = @@stderr_is_blocking
end
end

# Main function that acts as C's main function.
Expand Down
17 changes: 17 additions & 0 deletions src/io/file_descriptor.cr
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,23 @@ class IO::FileDescriptor < IO
end
end

# :nodoc:
def self.from_stdio(fd)
# If we have a TTY for stdin/out/err, it is a shared terminal.
# We need to reopen it to use O_NONBLOCK without causing other programs to break

# Figure out the terminal TTY name. If ttyname fails we have a non-tty, or something strange.
path = uninitialized UInt8[256]
if LibC.ttyname_r(fd, path, 256) == 0
# Open a fresh handle to the TTY
fd = LibC.open(path, LibC::O_RDWR)

new(fd).tap(&.close_on_exec = true)
else
new(fd, blocking: true)
end
end

def blocking
system_blocking?
end
Expand Down
11 changes: 5 additions & 6 deletions src/kernel.cr
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
{% if flag?(:win32) %}
STDIN = IO::FileDescriptor.new(0)
STDOUT = (IO::FileDescriptor.new(1)).tap { |f| f.flush_on_newline = true }
STDERR = (IO::FileDescriptor.new(2)).tap { |f| f.flush_on_newline = true }
STDOUT = IO::FileDescriptor.new(1).tap { |f| f.flush_on_newline = true }
STDERR = IO::FileDescriptor.new(2).tap { |f| f.flush_on_newline = true }
{% else %}
require "c/unistd"

STDIN = IO::FileDescriptor.new(0, blocking: LibC.isatty(0) == 0)
STDOUT = (IO::FileDescriptor.new(1, blocking: LibC.isatty(1) == 0)).tap { |f| f.flush_on_newline = true }
STDERR = (IO::FileDescriptor.new(2, blocking: LibC.isatty(2) == 0)).tap { |f| f.flush_on_newline = true }
STDIN = IO::FileDescriptor.from_stdio(0)
STDOUT = IO::FileDescriptor.from_stdio(1).tap { |f| f.flush_on_newline = true }
STDERR = IO::FileDescriptor.from_stdio(2).tap { |f| f.flush_on_newline = true }
{% end %}

PROGRAM_NAME = String.new(ARGV_UNSAFE.value)
Expand Down Expand Up @@ -445,7 +445,6 @@ def exit(status = 0) : NoReturn
status = AtExitHandlers.run status
STDOUT.flush
STDERR.flush
Crystal.restore_blocking_state
Process.exit(status)
end

Expand Down
1 change: 1 addition & 0 deletions src/lib_c/aarch64-linux-gnu/c/unistd.cr
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ lib LibC
fun getpid : PidT
fun getppid : PidT
fun isatty(fd : Int) : Int
fun ttyname_r(fd : Int, buf : Char*, buffersize : SizeT) : Int
fun lchown(file : Char*, owner : UidT, group : GidT) : Int
fun link(from : Char*, to : Char*) : Int
fun lockf(fd : Int, cmd : Int, len : OffT) : Int
Expand Down
1 change: 1 addition & 0 deletions src/lib_c/aarch64-linux-musl/c/unistd.cr
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ lib LibC
fun getpid : PidT
fun getppid : PidT
fun isatty(x0 : Int) : Int
fun ttyname_r(fd : Int, buf : Char*, buffersize : SizeT) : Int
fun lchown(x0 : Char*, x1 : UidT, x2 : GidT) : Int
fun link(x0 : Char*, x1 : Char*) : Int
fun lockf(x0 : Int, x1 : Int, x2 : OffT) : Int
Expand Down
1 change: 1 addition & 0 deletions src/lib_c/amd64-unknown-openbsd/c/unistd.cr
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ lib LibC
fun getpid : PidT
fun getppid : PidT
fun isatty(x0 : Int) : Int
fun ttyname_r(fd : Int, buf : Char*, buffersize : SizeT) : Int
fun lchown(x0 : Char*, x1 : UidT, x2 : GidT) : Int
fun link(x0 : Char*, x1 : Char*) : Int
fun lockf(x0 : Int, x1 : Int, x2 : OffT) : Int
Expand Down
1 change: 1 addition & 0 deletions src/lib_c/arm-linux-gnueabihf/c/unistd.cr
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ lib LibC
fun getpid : PidT
fun getppid : PidT
fun isatty(fd : Int) : Int
fun ttyname_r(fd : Int, buf : Char*, buffersize : SizeT) : Int
fun lchown(file : Char*, owner : UidT, group : GidT) : Int
fun link(from : Char*, to : Char*) : Int
fun lockf(fd : Int, cmd : Int, len : OffT) : Int
Expand Down
1 change: 1 addition & 0 deletions src/lib_c/i686-linux-gnu/c/unistd.cr
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ lib LibC
fun getpid : PidT
fun getppid : PidT
fun isatty(fd : Int) : Int
fun ttyname_r(fd : Int, buf : Char*, buffersize : SizeT) : Int
fun lchown(file : Char*, owner : UidT, group : GidT) : Int
fun link(from : Char*, to : Char*) : Int
fun lockf(fd : Int, cmd : Int, len : OffT) : Int
Expand Down
1 change: 1 addition & 0 deletions src/lib_c/i686-linux-musl/c/unistd.cr
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ lib LibC
fun getpid : PidT
fun getppid : PidT
fun isatty(x0 : Int) : Int
fun ttyname_r(fd : Int, buf : Char*, buffersize : SizeT) : Int
fun lchown(x0 : Char*, x1 : UidT, x2 : GidT) : Int
fun link(x0 : Char*, x1 : Char*) : Int
fun lockf(x0 : Int, x1 : Int, x2 : OffT) : Int
Expand Down
1 change: 1 addition & 0 deletions src/lib_c/x86_64-linux-gnu/c/unistd.cr
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ lib LibC
fun getpid : PidT
fun getppid : PidT
fun isatty(fd : Int) : Int
fun ttyname_r(fd : Int, buf : Char*, buffersize : SizeT) : Int
fun lchown(file : Char*, owner : UidT, group : GidT) : Int
fun link(from : Char*, to : Char*) : Int
fun lockf(fd : Int, cmd : Int, len : OffT) : Int
Expand Down
1 change: 1 addition & 0 deletions src/lib_c/x86_64-linux-musl/c/unistd.cr
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ lib LibC
fun getpid : PidT
fun getppid : PidT
fun isatty(x0 : Int) : Int
fun ttyname_r(fd : Int, buf : Char*, buffersize : SizeT) : Int
fun lchown(x0 : Char*, x1 : UidT, x2 : GidT) : Int
fun link(x0 : Char*, x1 : Char*) : Int
fun lockf(x0 : Int, x1 : Int, x2 : OffT) : Int
Expand Down
1 change: 1 addition & 0 deletions src/lib_c/x86_64-macosx-darwin/c/unistd.cr
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ lib LibC
fun getpid : PidT
fun getppid : PidT
fun isatty(x0 : Int) : Int
fun ttyname_r(fd : Int, buf : Char*, buffersize : SizeT) : Int
fun lchown(x0 : Char*, x1 : UidT, x2 : GidT) : Int
fun link(x0 : Char*, x1 : Char*) : Int
fun lockf(x0 : Int, x1 : Int, x2 : OffT) : Int
Expand Down
1 change: 1 addition & 0 deletions src/lib_c/x86_64-portbld-freebsd/c/unistd.cr
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ lib LibC
fun getpid : PidT
fun getppid : PidT
fun isatty(x0 : Int) : Int
fun ttyname_r(fd : Int, buf : Char*, buffersize : SizeT) : Int
fun lchown(x0 : Char*, x1 : UidT, x2 : GidT) : Int
fun link(x0 : Char*, x1 : Char*) : Int
fun lockf(x0 : Int, x1 : Int, x2 : OffT) : Int
Expand Down
7 changes: 4 additions & 3 deletions src/process.cr
Original file line number Diff line number Diff line change
Expand Up @@ -391,9 +391,10 @@ class Process

# :nodoc:
protected def self.exec_internal(command : String, argv, env, clear_env, input, output, error, chdir)
reopen_io(input, STDIN, "r")
reopen_io(output, STDOUT, "w")
reopen_io(error, STDERR, "w")
# Reopen handles if the child is being redirected
reopen_io(input, IO::FileDescriptor.new(0, blocking: true), "r")
reopen_io(output, IO::FileDescriptor.new(1, blocking: true), "w")
reopen_io(error, IO::FileDescriptor.new(2, blocking: true), "w")

ENV.clear if clear_env
env.try &.each do |key, val|
Expand Down
4 changes: 0 additions & 4 deletions src/signal.cr
Original file line number Diff line number Diff line change
Expand Up @@ -200,8 +200,6 @@ module Crystal::Signal
end

private def self.fatal(message : String)
Crystal.restore_blocking_state

STDERR.puts("FATAL: #{message}, exiting")
STDERR.flush
LibC._exit(1)
Expand Down Expand Up @@ -269,8 +267,6 @@ end

# :nodoc:
fun __crystal_sigfault_handler(sig : LibC::Int, addr : Void*)
Crystal.restore_blocking_state

# Capture fault signals (SEGV, BUS) and finish the process printing a backtrace first
LibC.dprintf 2, "Invalid memory access (signal %d) at address 0x%lx\n", sig, addr
CallStack.print_backtrace
Expand Down

0 comments on commit 2530bb6

Please sign in to comment.