libnative does not handle fds with O_NONBLOCK

[rust-highfive]

Issue by kballard
Saturday Apr 05, 2014 at 07:13 GMT
Originally opened as rust-lang/rust#13336

---

libnative does not detect when a read/write fails due to O_NONBLOCK being set on the fd. It makes the assumption that all of its files never have that flag set, because it never sets that flag on them. Unfortunately, this isn't necessarily the case. FIFOs and character device files (e.g. terminals) will actually share the O_NONBLOCK flag among all processes that have the same open file description (e.g. the underlying kernel object that backs the fd).

Using a tiny C program that uses fcntl() to set O_NONBLOCK on its stdout, a FIFO, and a Rust program that writes 32k of output to stdout, I can reproduce this issue 100% of the time. The invocation looks like

> (./mknblock; ./rust_program) > fifo

and on the reading side I just do

> (sleep 1; cat) < fifo

This causes the rust program to return a "Resource temporarily unavailable" error from stdout.write() after writing 8k of output. Removing the call to ./mknblock restores the expected behavior where the rust program will block until the reading side has started consuming input. And further, switching the rust program over to libgreen also causes it to block even with ./mknblock.

---

The C program looks like this:

#include <fcntl.h>
#include <stdio.h>

int main() {
    if (fcntl(1, F_SETFL, O_NONBLOCK) == -1) {
        perror("fcntl");
        return 1;
    }
    return 0;
}

The Rust program is a bit longer, mostly because it prints out information about stdout before it begins writing. It looks like this:

extern crate green;
extern crate rustuv;

use std::io;
use std::io::IoResult;
use std::libc;
use std::os;
use std::mem;

static O_NONBLOCK: libc::c_int = 0x0004;
static O_APPEND: libc::c_int = 0x0008;
static O_ASYNC: libc::c_int = 0x0040;

static F_GETFL: libc::c_int = 3;

unsafe fn print_flags(fd: libc::c_int) -> IoResult<()> {
    let mut stat: libc::stat = mem::uninit();
    if libc::fstat(fd, &mut stat) < 0 {
        try!(writeln!(&mut io::stderr(), "fstat: {}", os::last_os_error()));
        libc::exit(1);
    }

    try!(writeln!(&mut io::stderr(), "stdout: dev={}, ino={}", stat.st_dev, stat.st_ino));

    let flags = libc::fcntl(fd, F_GETFL);
    if flags == -1 {
        try!(writeln!(&mut io::stderr(), "fcntl: {}", os::last_os_error()));
        libc::exit(1);
    }

    let mut v = Vec::new();
    if flags & O_NONBLOCK != 0 {
        v.push("nonblock");
    }
    if flags & O_APPEND != 0 {
        v.push("append");
    }
    if flags & O_ASYNC != 0 {
        v.push("async");
    }

    try!(writeln!(&mut io::stderr(), "flags: {}", v.connect(", ")));
    Ok(())
}

fn run() -> IoResult<()> {
    unsafe { try!(print_flags(1)); }

    let mut out = io::stdio::stdout_raw();
    for i in range(0u, 32) {
        try!(writeln!(&mut io::stderr(), "Writing chunk {}...", i));
        let mut buf = ['x' as u8, ..1024];
        buf[1023] = '\n' as u8;
        match out.write(buf) {
            Ok(()) => (),
            Err(e) => {
                try!(writeln!(&mut io::stderr(), "Error writing chunk {}", i));
                return Err(e);
            }
        }
    }
    Ok(())
}

fn main() {
    match run() {
        Err(e) => {
            (writeln!(&mut io::stderr(), "Error: {}", e)).unwrap();
            os::set_exit_status(1);
        }
        Ok(()) => ()
    }
}

unsafe fn arg_is_dash_g(arg: *u8) -> bool {
    *arg == '-' as u8 &&
        *arg.offset(1) == 'g' as u8 &&
        *arg.offset(2) == 0
}

#[start]
fn start(argc: int, argv: **u8) -> int {
    if argc > 1 && unsafe { arg_is_dash_g(*argv.offset(1)) } {
        green::start(argc, argv, rustuv::event_loop, main)
    } else {
        native::start(argc, argv, main)
    }
}

[rust-highfive]

Comment by kballard
Saturday Apr 05, 2014 at 07:15 GMT

---

The two solutions I can think of for this that seem reasonable are:

1. When a read/write returns EAGAIN or EWOULDBLOCK, fall back to a call to select().
2. When a read/write returns EAGAIN or EWOULDBLOCK, use fcntl() to turn off O_NONBLOCK and try again.

[rust-highfive]

Comment by huonw
Saturday Apr 05, 2014 at 07:27 GMT

---

cc @alexcrichton

[rust-highfive]

Comment by alexcrichton
Saturday Apr 05, 2014 at 22:11 GMT

---

Surely we can't be the first project/language that has run in to this. I would be curious what other languages/runtimes do in the face of this error.

I don't think that libuv handles this test case specifically, I think it just falls out of the general architecture of libuv.

[rust-highfive]

Comment by kballard
Saturday Apr 05, 2014 at 22:13 GMT

---

@alexcrichton I suspect other projects/languages either don't have an opinion on O_NONBLOCK and expect the clients of the I/O APIs to deal with it, or they break. I'm curious if any explicitly address this problem.

I have a tentative fix over at kballard/rust/libnative_io_nonblock, although I suspect that hardcoding the values for F_GETFL/F_SETFL/O_NONBLOCK isn't portable and I'd like to find out how to get libc to contain these values.

[rust-highfive]

Comment by alexcrichton
Saturday Apr 05, 2014 at 22:19 GMT

---

Hm, regardless of the values of the flags, I'm not sure that's the best solution. It's not guaranteed that every file descriptor passed in to libnative wants blocking reads/writes. In theory you could pass one in, and then have a select/epoll loop running somewhere else.

So far this problem seems isolated to only the stdout/stderr file descriptors, so I think those are the only ones that should be modified.

[rust-highfive]

Comment by kballard
Saturday Apr 05, 2014 at 22:23 GMT

---

@alexcrichton I don't see how you can restrict it to only stdout/stderr. Those can be dup'd or redirected, so you'd have to have some way, inside of inner_read() or inner_write() to unambiguously detect that this fd is actually stdout or stderr, and modify them appropriately. Modifying it at any other time opens you to the fd changing again (e.g. by another process modifying the tty device back to O_NONBLOCK). And I don't think you can unambiguously detect this anyway (certainly checking if fd == 1 isn't sufficient because it could have been dup'd).

The alternative implementation I considered was to fall back to select() instead of turning off O_NONBLOCK, but I thought this was a better solution because in most cases it's only going to be triggered once (assuming the problem exists in the first place), but the select() approach would end up falling back to select() quite often.

If, in Rust code, you want to deal with non-blocking I/O, I think you need to roll your own on top of libc. At least, until such time as someone comes up with a good proposal for non-blocking I/O in libnative/libgreen. But for the moment, clients of libnative (and libgreen) expect their read() and write() calls to be blocking.

[rust-highfive]

Comment by kballard
Thursday Apr 24, 2014 at 18:55 GMT

---

This is not fixed. #13619 may fix a common way to trigger the issue, but my test code reproduces the issue without requiring libgreen.

[rust-highfive]

Comment by alexcrichton
Thursday Apr 24, 2014 at 22:13 GMT

---

Sorry, I meant to comment on this soon after the PR landed, and I was in a meeting.

I do not believe that this falls under the prerogative of libnative. Dealing with nonblocking file descriptors is clearly a tricky situation (as seen with libuv) that I do not believe is up to us. Only owned file descriptors should have the nonblocking bits fiddled with, and all of the file descriptors we own are guaranteed to be blocking.

[rust-highfive]

Comment by kballard
Thursday Apr 24, 2014 at 22:22 GMT

---

@alexcrichton Why do you believe this is something libnative shouldn't handle? As I've demonstrated, certain file descriptors are shared among multiple processes, which means that libnative could encounter a nonblocking fd even when it thinks it owns the fd (e.g. if you open a fifo, the other process using that fifo can mark it nonblock).

all of the file descriptors we own are guaranteed to be blocking

And this is precisely why I believe so strongly that libnative must deal with this issue. libnative guarantees that file descriptors it manages are done so in a blocking fashion. Therefore, when it encounters a file descriptor that has been marked as nonblock by another process, it must deal with this, or it will break its own guarantee.

[rust-highfive]

Comment by alexcrichton
Thursday Apr 24, 2014 at 22:24 GMT

---

Why do you believe this is something libnative shouldn't handle

The code you're proposing is the exact code that caused the bug from libuv. That, plus the overwhelming precedence for not handling this is why I believe this is unnecessary.

In summary: Yes, this can happen. No, we should not handle it specially. Our behavior now, returning that the I/O operation failed, is correct in my opinion.

[rust-highfive]

Comment by thestinger
Thursday Apr 24, 2014 at 22:26 GMT

---

A program setting O_NONBLOCK on a file descriptor shared with processes it doesn't control is buggy. There's no sane way to deal with it, since removing it will break a guarantee expected by some other process. Trying to remove it and add it back before anyone notices is a racy hack.

https://lkml.org/lkml/2007/8/14/135

@alexcrichton: I agree that considering it an error is the best solution. Does it actually return EAGAIN as the error so you know there's another buggy process involved?

[rust-highfive]

Comment by kballard
Thursday Apr 24, 2014 at 22:31 GMT

---

@alexcrichton

The code you're proposing is the exact code that caused the bug from libuv

How can that possibly be true? My code is removing the O_NONBLOCK when it encounters it. Surely libuv is not doing that.

As for the overwhelming precedence for not handling this, I think that the languages you've been looking at are wrong. Additionally I don't think they chose to not handle this, I think they just aren't aware the bug exists in the first place. Plus, I believe at least some of them surface the underlying fcntl() mechanism which would allow a client to solve this problem. Rust does not (at least, not from std::io's API).

@thestinger The POSIX non-blocking I/O stuff is horribly broken. There's no way around that. Any process that relies on O_NONBLOCK with a shared file description is already rather broken, because there's nothing stopping other programs from turning that off. But this isn't a good reason to avoid turning off O_NONBLOCK. And we definitely don't try to "add it back before anyone notices". We just leave it off.

An alternative fix would be to use select() to block on the fd instead (which I assume ignores the nonblocking nature of the fd but I haven't researched that), but I thought that was a) an uglier solution, and b) triggers the workaround on every single read/write from the fd, whereas turning off O_NONBLOCK will presumably only ever have to happen once.

[rust-highfive]

Comment by thestinger
Thursday Apr 24, 2014 at 22:34 GMT

---

Turning it off is masking a very real programming error instead of uncovering the problem. The process expecting O_NONBLOCK to be there is now going to be screwed up, without any indication of what's going on. It's far better to report the error. It could easily be set again before you actually perform a read or write to the file descriptor... it's best to report the error while you can.

[rust-highfive]

Comment by alexcrichton
Thursday Apr 24, 2014 at 22:35 GMT

---

FYI, libuv set stdin to nonblocking, which also set stdout to nonblocking. That is the bug I'm referring to. Flipping flags on a descriptor you don't own will result in bugs (as was just proven by libuv).

[rust-highfive]

Comment by kballard
Thursday Apr 24, 2014 at 22:39 GMT

---

@thestinger Any process expecting O_NONBLOCK to persist on a file descriptor that represents a shared resource is already broken. There's no sense in trying to coddle it. And the Rust code that's calling read/write has no possible solution if you report an error. It can't turn off O_NONBLOCK because it's presumably using std::io and therefore has no access to the underlying fd. Trying to report an error here is basically saying "here's a transient error, your choices are either to treat it as a permanent unrecoverable error, which is crap, or retry in a tight loop, which is also crap". The only sane solution is for libnative to handle the error itself, because that's the only place the error can be handled.

@alexcrichton Yes, it sounds like libuv was being stupid. I take it upgrading libuv made it stop doing that? However, stdin/stdout/stderr are expected to be blocking by default, and so I don't see the problem with libnative restoring the blocking behavior if they somehow get set to non-blocking (e.g. by a separate process).

[rust-highfive]

Comment by thestinger
Thursday Apr 24, 2014 at 22:40 GMT

---

The error can't be handled; it can only be masked. It is not fixing the underlying problem and will cause hidden breakage elsewhere. When an error happens and there's no sane way to handle it internally, it should be reported. This allows the logic error to be discovered and the source of the bug can be fixed.

[rust-highfive]

Comment by kballard
Thursday Apr 24, 2014 at 22:42 GMT

---

@thestinger No, it can be handled. Turning off non-blocking is not masking. It's the proper solution to an fd that is unexpectedly nonblocking. The fundamental bug lies in whatever process decided to mark a shared file description as non-blocking and expecting it to stick. And there's nothing Rust can do about that.

Again, "reporting" the error means telling the client that it needs to give up and consider this an unrecoverable error, which is pretty awful when it is not, in fact, an unrecoverable error. It's a pretty trivially recoverable error, but libnative just refused to recover from it.

[rust-highfive]

Comment by thestinger
Thursday Apr 24, 2014 at 22:44 GMT

---

Rust should report the logic error so it can be fixed. The other process is not going to be able to cope with the file descriptor becoming blocking if it expected it to be non-blocking. It will break in mysterious ways and the painful debugging process with be Rust's fault for trying to correct the mistake. Attempting to remove the flag is a race, because a process setting the flag is very likely to keep setting it whenever it does a read/write. There's no way to know how long it will remain unset and the consequences for the other process. The programmer may never encounter the issue because Rust was masking it, and it could very easily cause data loss for users.

[rust-highfive]

Comment by kballard
Thursday Apr 24, 2014 at 22:49 GMT

---

@thestinger Why do you keep saying this? It's not an error in Rust. Trying to "report" it does not allow for any possible way to fix it. The bug is in whatever other process marked the file description as non-blocking. What you're insisting on doing is creating a bug in Rust in an attempt to try and avoid breaking an already-broken unknown separate program.

[rust-highfive]

Comment by kballard
Thursday Apr 24, 2014 at 22:50 GMT

---

For reference, not fixing this in Rust says absolutely nothing about whether another program will decide to turn off O_NONBLOCK for its own reasons (maybe it turns on O_NONBLOCK for one read, then turns it off again for a second read). This will break the unknown non-blocking program just as much as implementing this fix in Rust will.

[rust-highfive]

Comment by thestinger
Thursday Apr 24, 2014 at 22:55 GMT

---

It could very well be an error in the Rust process. It's perfectly sane to intentionally share a non-blocking file descriptor with a child you always control.

This will break the unknown non-blocking program just as much as implementing this fix in Rust will.

That's why Rust should report an error as soon as possible. It found a horrible problem needing attention from a programmer. There's really no limit to what could go wrong if it's left masked.

[rust-highfive]

Comment by thestinger
Thursday Apr 24, 2014 at 22:57 GMT

---

Trying to "report" it does not allow for any possible way to fix it. The bug is in whatever other process marked the file description as non-blocking. What you're insisting on doing is creating a bug in Rust in an attempt to try and avoid breaking an already-broken unknown separate program.

It doesn't really matter where the bug originates. If Rust is interacting with a program that's buggy over something like a pipe, it's a bug in the Rust program if they're unable to work correctly together. The other program may even document that the child receives a non-blocking file descriptor and needs to handle it correctly.

[rust-highfive]

Comment by kballard
Thursday Apr 24, 2014 at 23:21 GMT

---

@thestinger I cannot possibly fathom how you can believe that a third-party process that sets a shared file description as non-blocking can be considered a bug in Rust. But that's exactly what you're proposing. What's more, your proposed resolution, of "reporting" the bug, leaves the client code with no possible solution aside from discarding the file descriptor entirely as if it has an unrecoverable error. There is no sane world in which a bug in an arbitrary third-party program can possibly be fixed by your program written in Rust receiving this error report. And there's no sane world in which makes sense to introduce bad behavior in your Rust program as a result of a bug in a third-party program which you can diagnose and fix but intentionally chose not to. That just doesn't make sense. The third-party program is already fundamentally broken. Why do you insist on propagating the bug into Rust?

If Rust is interacting with a program that's buggy over something like a pipe, it's a bug in the Rust program if they're unable to work correctly together.

Besides the fact that pipes don't have this bug (FIFOs and TTYs do, but not pipes), this makes no sense. CLI programs interacting with each other over some file descriptor channel do not use the non-blocking nature of the fd as part of the protocol, only the actual contents of the sent data.

The only scenario you've mentioned that makes any sense at all is intentionally sharing a non-blocking fd with a spawned child, but I don't see how that's relevant. Rust does not support fd-based non-blocking I/O. Period. Hopefully we will have a solution to non-blocking I/O at some point, but we don't today, and libnative's current implementation of I/O is very explicitly blocking. If you take this shared fd and hand it to libnative, then you are very explicitly asking for blocking behavior. As such, it is reasonable to assume that libnative will turn off O_NONBLOCK at it discretion.

In fact, it would be perfectly reasonable for libnative, upon receiving a new fd, to immediately check the status flags of the fd and turn off O_NONBLOCK if it finds it on. Not in response to EWOULDBLOCK, but as part of the initialization of its FileDesc. This would be very much in keeping with libnative being designed for blocking I/O, and believing that you handing it an fd means you're requesting it to set up the fd properly for said blocking I/O. This behavior would cause problems for third-party programs just as much as my solution to this bug, but it seems to me to be more obviously appropriate behavior for Rust to do. Of course, I'm not suggesting we do this, because it doesn't actually fix the bug at all (as the fd can be marked as nonblocking again later by the third-party program).

[rust-highfive]

Comment by kballard
Thursday Apr 24, 2014 at 23:22 GMT

---

Fundamentally, non-blocking I/O in POSIX is completely broken. Rust's goal here should be to produce the expected behavior for the Rust program. This behavior means blocking I/O when using libnative. It does not matter if this exposes a bug in a third-party program. That program was already broken, it just didn't know it yet.

[rust-highfive]

Comment by thestinger
Sunday May 04, 2014 at 22:35 GMT

---

It's fine to set O_NONBLOCK on a shared file descriptor when all processes that are going to be involved are designed to handle it. That's why Rust should report an error when the file descriptor is not set up as it's expected to be. It's either a bug in the process setting the flag or a bug in the Rust program, but it's certainly a logic error and should be reported as such. It's not a runtime error Rust should attempt to recover from.

[rust-highfive]

Comment by kballard
Sunday May 04, 2014 at 23:43 GMT

---

@thestinger I still don't understand this argument. What do you expect the caller to do when libnative returns the error? There is literally no way for the calling program to handle this error gracefully.

As I have said before, I consider the bug to be in the program that set O_NONBLOCK on the shared fd. Reporting the error in the Rust program is not helpful, because the Rust program has no way of influencing the actually buggy program.

[rust-highfive]

Comment by thestinger
Sunday May 04, 2014 at 23:47 GMT

---

The Rust program could be the buggy one if it's expected that the shared file description is O_NONBLOCK. It's only wrong to set it if there are processes not under your control sharing the file descriptor.

[rust-highfive]

Comment by kballard
Monday May 05, 2014 at 00:03 GMT

---

You've said that before, and I truly do not understand what you are trying to say. We're not talking here about fds that are being given to a Rust program either through forking, or through a unix socket. The only way for those fds to end up being touched by libnative is if you explicitly wrap them in a native::io::file::FileDesc, and doing so is an explicit request that this fd be handled in a blocking fashion (which means that for libnative to not do so, that is rightly considered a bug in libnative). Taking an fd that is intended to be O_NONBLOCK and handing it to libnative would indeed be a bug in the calling program, but that's irrelevant here because that's an API misuse bug, which is not solved by deliberately leaving bugs in the API implementation.

No, what we are talking about is file descriptions that are shared by virtue of being either a tty or a fifo (and it's plausible there's another type of file that behaves like this as well), that end up hooked up to the Rust programs stdin, stdout, or stderr. There is no circumstance in which it is justifiable to impose O_NONBLOCK on a separate process's stdin/stdout/stderr. Doing so is strictly a bug in the program that set O_NONBLOCK. And this is true even if you wrote both the Rust program and the program that set O_NONBLOCK. If the Rust program wants O_NONBLOCK then it should set it itself and avoid using libnative's file handling. And it would be far more appropriate to pass the FIFO as an argument to the Rust program instead of hooking its stdin/stdout up to it. However, even talking about FIFOs is a bit distracting, because they're rather rare to use and even rarer to attach to stdin/stdout. The real issue is when a tty is set to O_NONBLOCK and it never makes sense to do that.

---

In any case, even talking about whether O_NONBLOCK was intentional is I think completely beside the point. The fact is, libnative is documented as a blocking I/O API. The bug is that this guarantee is not actually enforced.

[rust-highfive]

Comment by thestinger
Monday May 05, 2014 at 01:19 GMT

---

It's being enforced by reporting an error if the file descriptor has incorrect flags and requiring that the calling code handle the error via the unused result lint.

Setting this for stdin, stdout or stderr is not really wrong if you have control of both ends and can guarantee there's nothing expecting it to be blocking.

I don't really see how it's a bug as-is. It's a decision to report the error instead of trying to fix it by changing O_NONBLOCK. This is unlikely to turn out well since some process did set this, and expects it to hold.

[rust-highfive]

Comment by kballard
Monday May 05, 2014 at 01:58 GMT

---

@thestinger What part of "the calling code has no possible way of handling the error" are you not understanding? The only way the calling code can possibly handle the error is if it's using libnative directly (which it's probably not, it's probably going through std::io) and falling back to libc functions. Except of course the functions it needs aren't even exposed by liblibc at the moment (they were added by my PR). And since it's probably using std::io it's moot anyway.

Setting this for stdin, stdout or stderr is not really wrong if you have control of both ends and can guarantee there's nothing expecting it to be blocking.

The only way this is correct is if you're using a FIFO, which is extremely rare. And if you are using a FIFO, and have control over both ends, then you're not even going to run into this problem in the first place because you won't be using libnative (because libnative is blocking I/O and you just stated that you're intentionally using non-blocking). Since you're not using libnative in this scenario, it's not at all applicable to this issue.

The reason why that's the only way this is correct is because pipes don't exhibit this issue (pipes ends do not share their file description), and it's unambiguously a bug to set a tty to O_NONBLOCK (as you cannot guarantee you control all processes attached to the tty).

This is unlikely to turn out well since some process did set this, and expects it to hold.

I've stated repeatedly that the process that did this is buggy by definition. Causing the Rust program to be buggy too in an attempt to avoid breaking the already-buggy third-party program makes no sense to me. Why do you think that's a rational thing to do?

[nrc]

This was just a test for issue importing...

[thestinger]

wat

[nrc]

sorry for the spam :-(

I'll be moving some issues from rust to rfcs and this was a test run.