Close logpipe input file descriptor after dup2()

[MarijnS95]

When the input file descriptor of the pipe() is dup2()'d into stdin and stdout it is effectively copied, leaving the original file descriptor open and leaking at the end of these statements. Only the output file descriptor has its ownership transferred to File and will be cleaned up properly.

This should cause the reading end to read EOF and return zero bytes when stdin and stdout is open, rather than remaining open "indefinitely" (barring the whole process being taken down) as there will always be that one file descriptor referencing the input end of the pipe.

[MarijnS95]

We should probably drive-by fix the error handling, because this will loop indefinitely as long as read_line() returns an error. For EOF it returns Ok(0) at least 😌

[rib]

It looks like that EOF case is handled afterwards with if len == 0 { break; }? aah right, you're saying at least the EOF case is OK but it doesn't handle errors! Yeah that's not great!

At least it looks like ErrorKind::Interrupted is hidden from callers, so I guess probably any io::Error here will be a legitimate error that means we should exit the thread.

[MarijnS95]

Yeah that's exactly the Ok(0), we handle that, but there's nothing handling Err from read_line(). If that's returned indefinitely, this'll get a CPU core quite busy.

[MarijnS95]

Using OwnedFd might be even nicer: it'll close the file descriptors automatically and has a (safe) impl From<OwnedFd> for File.

Even though it is FFI-compatible I'm not sure if we're allowed to rely on the fact that they are initially uninitialized, or perhaps libc::pipe() not initializing them at all or to -1 on error?

[MarijnS95]

Actually I think we should switch to nix or rustix: no code below is validating the error codes returned by libc, while those APIs do.

On quick inspection I prefer rustix: they return OwnedFd from pipe() rather than nix' returning RawFd.

[MarijnS95]

That's pretty cool:

        let file = unsafe {
            let logpipe = rustix::pipe::pipe().unwrap();
            rustix::stdio::dup2_stdout(logpipe.1);
            rustix::stdio::dup2_stderr(logpipe.1);

            File::from(logpipe.0)
        };

Unfortunately converting this whole file to rustix is a hassle :(

[MarijnS95]

Ping @rib :)

[rib]

I'm half wondering whether we should be setting O_CLOSEXEC via pipe2() also but actually I suppose it would maybe make sense that if you fork + exec for some reason then you'd want stdout/err for the thing you exec to also get directed to this same IO thread.

I guess it'd make sense to use O_CLOSEXEC on the just the read end maybe.

[MarijnS95]

@sunfishcode hence perhaps using rustix in a popular boilerplate Rust+Android crate will give it the testing it deserves?

We even used to run ndk-glue in an emulator, but I don't think that was ported to android-activity.

[MarijnS95]

Anyway, let's move/continue discussions to #139 if there needs to be any.

[rib]

I think it's more like you had have a ref counted file object and a separate file descriptor - which is relevant here since the flags are associated with the fd, not the thing that's ref-counted.

Exactly, that is why the O_CLOEXEC no longer has an effect when the write-end is dup2()'d into stdout/stderr.

(nit: note that there's no S in there, ie. O_CLOseonEXECute)

thanks, yeah, I guess I forgot how many letters from 'close' to keep - I'd guess even that the 'O' might be for 'on', who knows :)

Since I think dup2 will create new FDs for stdout/err that don't have O_CLOSEXEC by default then it looks like we do now have the final result we want.

dup2() can't, because you're telling the kernel to make the stdout/stderr FD to "point to" the underlying (refcounted) object described/pointed-to by the "new" FD created by pipe(). If it were to create and assign new FDs things would break and pretty much defeat the purpose of dup2()?

hmm, no, dup2() doesn't really make an existing file descriptor point to the same file object - though it does give the illusion that that's what it does.

dup2() here effectively (atomically) close()s the original stdin/err (deleting the original FDs) and then creates new FDs that don't have flags set (such as O_CLOEXEC). The new FD created by dup2 hold a reference to the same file object that the oldfd points at.

So if you think of an FD as only the integer then it does give that illusion that an existing FD is being made to point at another file - but the FD really has some associated state (including the flags such as O_CLOEXEC) so it's quite an important distinction to note that dup2 is creating a new FD and the new stdin/err file descriptors from dup2() won't have O_CLOEXEC set.

This is effectively why there's now a dup3() to close the same race condition that existed with other system calls that create new file descriptors without O_CLOEXEC already set.

From https://man7.org/linux/man-pages/man2/dup.2.html:

The dup2() system call performs the same task as dup(), but instead of using the lowest-numbered unused file descriptor, it uses the file descriptor number specified in newfd. In other words, the file descriptor newfd is adjusted so that it now refers to the same open file description as oldfd.

The wording is very particular here and the devil is in the details. It's saying it "uses the file descriptor number", not the actual, pre-existing file descriptor. An FD is more than just the "number" that gets passed around in userspace and the FD is something that's also separate from the file object that it references.

Some file state is common to the referenced file object and some other state like the O_CLOEXEC is unique to each FD.

dup2() closes one FD, then creates a new FD that will also use the same number (such as stdout/err) that was previously associated with the FD which has been closed - and then makes that new FD reference the same file object as the other FD passed to dup2().

I'd maybe be slightly nervous about how well tested rustix is on Android in terms of being aware of what system calls are allowed on Android. At least with libc it tends to pretty much have a 1:1 mapping and it's easy to avoid system calls that aren't allowed.

It's not like rustix seems to call "arbitrary" syscalls under the hood at all, or abstract them away. Rather, it's providing better type safety around equally-named UNIX sycalls? At least for the ones that I've been looking at.

I guess at least the rustix binding for dup2 looked pretty explicit that it would map to the dup2 system call. I guess I would expect that not every API is implemented with such an explicit 1:1 mapping to system calls though and its those slightly higher-level APIs I'd maybe worry about.

[MarijnS95]

I didn't want to go to the depths of discussing that a light-weight file descriptor integer in userspace must (obviously) have a whole representation on the kernel side before even pointing to any sort of file object.

However, for a completely distinct case, it is perhaps worth mentioning it, starting with this quote from dup3() docs:

dup()/dup2()
...
The two descriptors do not share file descriptor flags (the close-on-exec flag). The close-on-exec flag (FD_CLOEXEC; see fcntl(2)) for the duplicate descriptor is off.

dup3() is the same as dup2(), except that:

• The caller can force the close-on-exec flag to be set for the new file descriptor by specifying O_CLOEXEC in flags. See the description of the same flag in open(2) for reasons why this may be useful.

We already deduced above that FD flags affect the FD (or FD number as it were), and won't be "copied" over from oldfd to newfd. However, noting that newfd gets its whole, for the theoretical case that stdin/stdout have O_CLOEXEC set, FD replaced without any sharing is still useful: in this case the resulting FD (which the FD number points to) won't have O_CLOEXEC set anymore.

---

As such, if there is a case where an Android app may set O_CLOEXEC on stdin/stderr before handing control over to android-activity, we'll undo that with our dup2(). Should we have F_GETFD to check if it is set, and if so use dup3() to add it back?

[MarijnS95]

I guess at least the rustix binding for dup2 looked pretty explicit that it would map to the dup2 system call. I guess I would expect that not every API is implemented with such an explicit 1:1 mapping to system calls though and its those slightly higher-level APIs I'd maybe worry about.

I'm not worried too much if we're translating existing libc:: calls to their 1:1 equivalents in rustix, unless we cannot find that or stumble upon a more high-level abstraction over it, in which case we will have to check whether it does the same.

However, I doubt android-activity does much if anything special with syscalls.

[rib]

Thanks for looking at this - it looks good to me!