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

Reopen standard handles when they are a TTY #6518

Merged
merged 8 commits into from Aug 20, 2018

Conversation

Projects
None yet
6 participants
@Timbus
Contributor

Timbus commented Aug 10, 2018

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,
and also prevents crystal from breaking itself when spawning a new process.

Two important things:

  1. This changes the std handle numbers. Anyone expecting their programs STDIN/OUT/ERR to be 0,1, and 2, will be (sorta) wrong. This shouldn't actually affect anyone? But if it does, we can save the original FDs and swap em around.
  2. There are two places where I am not dealing with potential error codes and would like to know how I'm supposed to handle that, especially if the output might not be available?

To test, I put these two files in the build directory, and ran tester.sh with bash

tester.sh

#!/usr/bin/env bash

./.build/crystal test.cr

read -p "Prompt: " result
printf '%s\n' "$result"

test.cr

state = STDOUT.blocking
puts "Do we block now? #{state}"

system("echo call a subprocess")

if STDOUT.blocking == state
  puts "We cleanly spawned =)"
else
  puts "ERRAR! Child messed with handles"
end

This checks that the child process doesn't mess with the handle, and also ensures crystal doesn't mess with the TTY's stdin.

@asterite

This comment has been minimized.

Contributor

asterite commented Aug 10, 2018

The file(s) that define StdFileDescriptor are missing.

# We need to reopen it to use O_NONBLOCK without causing other programs to break
if tty?
# Figure out the terminal TTY name
path = UInt8[256]

This comment has been minimized.

@asterite

asterite Aug 10, 2018

Contributor
path = uninitialized UInt8[256]

What you wrote creates a slice of size 1 with the value 256. You are lucky it doesn't somehow segfaults.

This comment has been minimized.

@Timbus

Timbus Aug 10, 2018

Contributor

Thanks, I misinterpreted this quote from the gitbook: "Static arrays (StaticArray(Int32, 8), which can also be written as Int32[8])". I get it now.

require "./syscall"
require "./file_descriptor"
class IO::StdFileDescriptor < IO::FileDescriptor

This comment has been minimized.

@asterite

asterite Aug 10, 2018

Contributor

I would probably avoid a subclass and just add a boolean in the constructor to do this small logic. But I'd like to know others' opinions.

This comment has been minimized.

@Timbus

Timbus Aug 10, 2018

Contributor

There was more code when I started out, trying to dup old FDs and stuff. But yeah this could be put straight into IO::FD I guess.

This comment has been minimized.

@straight-shoota

straight-shoota Aug 10, 2018

Contributor

You don't need a flag, just a differently named constructor, like FileDescriptor.standard which calls FileDescriptor.new with the newly opened fd.

This comment has been minimized.

@ysbaddaden

ysbaddaden Aug 10, 2018

Member

We don't need to test for tty? all the time either. A special constructor is a good idea.

@asterite

This comment has been minimized.

Contributor

asterite commented Aug 10, 2018

@Timbus

This comment has been minimized.

Contributor

Timbus commented Aug 10, 2018

Well its 1am, I'll pick this back up when I wake up.
Ah, for reference this was supposed to be an easier alternative to #6068 but .. we will see.

Timbus added some commits Aug 10, 2018

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'.
@asterite

This comment has been minimized.

Contributor

asterite commented Aug 10, 2018

I hope it works! It's really much simpler.

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.

@Timbus Timbus force-pushed the Timbus:fix-ttyio branch from 8679cc6 to 8d563a3 Aug 11, 2018

@Timbus

This comment has been minimized.

Contributor

Timbus commented Aug 11, 2018

So you probably wouldn't believe the combination of things I've tried in order to get everything working, but it turns out I've been trying to fix bugs that I didn't even introduce -_-

My battery of custom tests included a check to see if crystal can also handle piping to another program:
crystal eval 'puts "hi"' | bash -c 'sleep 5'

But I didn't realize that crystal can't handle a pipe closing:
crystal eval 'sleep 1; puts "hi"' | echo 'prepare to die!'

So it was a timing bug. Ugh I thought it was a regression.

@Timbus

This comment has been minimized.

Contributor

Timbus commented Aug 11, 2018

'Kay. I think it's good now.

It's outside the scope of the original issue, but I might look into handling closed descriptors better. I nearly managed to handle closed FDs:
crystal eval 'puts "bla"' >&-

But handling descriptors being closed dynamically would be more ideal, and would also solve the issue in my last comment.

@@ -18,6 +18,27 @@ class IO::FileDescriptor < IO
end
end
def self.from_stdio(fd)

This comment has been minimized.

@straight-shoota

straight-shoota Aug 11, 2018

Contributor

This method should probably be :nodoc:, it's only internal implementation and probably not useful in user code.

This comment has been minimized.

@Timbus

Timbus Aug 11, 2018

Contributor

👍

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 %}

This comment has been minimized.

@j8r

j8r Aug 11, 2018

Contributor

I would rather not use tap for win32 and repeat for unix (&. can be used btw), but put at the end

STDOUT.flush_on_newline = true
STDERR.flush_on_newline = true

This comment has been minimized.

@Timbus

Timbus Aug 11, 2018

Contributor

I don't know about const assignment in crystal, but is it potentially going to be run out-of-order if I do that? For example, it looked like you could use STDIN in the main method, before code in kernel.cr runs. (I could be wrong)

This comment has been minimized.

@j8r

j8r Aug 11, 2018

Contributor

Didn't know this detail about out of order execution.
Still, a style detail, |f| f. can be replaced by &. 😄

{% if flag?(:win32) %}
  STDOUT = IO::FileDescriptor.new(1).tap &.flush_on_newline = true
  STDERR = IO::FileDescriptor.new(2).tap &.flush_on_newline = true
{% else %}
  require "c/unistd"
  STDOUT = IO::FileDescriptor.from_stdio(1).tap &.flush_on_newline = true
  STDERR = IO::FileDescriptor.from_stdio(2).tap &.flush_on_newline = true
{% end %}
@asterite

This comment has been minimized.

Contributor

asterite commented Aug 11, 2018

That's correct. tap must be used.

@@ -18,6 +18,28 @@ class IO::FileDescriptor < IO
end
end
# :nodoc:
def self.from_stdio(fd)
# XXX: This is -supposed- to work, but something (libevent?) is changing the FD before here.

This comment has been minimized.

@RX14

RX14 Aug 12, 2018

Member

Is this comment still relevant? Is this PR still WIP?

This comment has been minimized.

@Timbus

Timbus Aug 12, 2018

Contributor

The code should still be there, and the comment still stands until someone (me?) looks into the epoll setup that happens for all handles once the first one is initialized. But I'd say the PR is ready

@RX14

This comment has been minimized.

Member

RX14 commented Aug 12, 2018

libuv dup3's the FD to replace the old one with the newly open one.

I'm not sure if that's required here, since the standard file descriptors remain open and referring to the same device.

@Timbus

This comment has been minimized.

Contributor

Timbus commented Aug 12, 2018

I initially did that, but then you also need to dup a copy of the original FD so you can pass it to the kid process and it was a bit messy. From what I see libuv always uses new pipes instead of sharing FDs with a child process, so it can afford to dup over the originals (a safer approach, imo)

@RX14

This comment has been minimized.

Member

RX14 commented Aug 12, 2018

@Timbus why do the child processes need the original FD? Surely the only thing required is to reset O_NONBLOCK before calling exec?

@Timbus

This comment has been minimized.

Contributor

Timbus commented Aug 12, 2018

That would be to solve the issue observed in #5830 .. Blocking IO on the shared FD halts the event loop. So we have to give the child the original FD and keep ours as nonblocking.

@RX14

This comment has been minimized.

Member

RX14 commented Aug 12, 2018

ohhh yeah, my bad, thanks for the correction!

@RX14

RX14 approved these changes Aug 14, 2018

@ysbaddaden

The change looks great and simple. I can't test effectiveness right now, but I will trust you if all failing snippets are now working correctly.

@RX14 RX14 added the kind:feature label Aug 20, 2018

@RX14 RX14 added the topic:stdlib label Aug 20, 2018

@RX14 RX14 added this to the 0.26.1 milestone Aug 20, 2018

@RX14 RX14 merged commit 2530bb6 into crystal-lang:master Aug 20, 2018

4 checks passed

ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

omarroth added a commit to omarroth/crystal that referenced this pull request Nov 5, 2018

Reopen standard handles when they are a TTY (crystal-lang#6518)
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment