Re: [RFC PATCH v2] setup: avoid unconditional open syscall with write flags

[Junio C Hamano <gitster@pobox.com>]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

>> +	fd = xopen("/dev/null", O_RDONLY);
>> +	if (fd > 0)
>> +		close(fd);
>> +	if (fd > 2)
>> +		return;
>> +
>> +	fd = xopen("/dev/null", O_WRONLY);
>>  	while (fd < 2)
>>  		fd = xdup(fd);
>>  	if (fd > 2)
>
> I don't really get this, if it's safe under seccomp to open this
> O_RDONLY wasn't it always redundant to use O_WRONLY, and this change
> should just be switching to O_RDONLY?

The first "if we successfully opened /dev/null and also found that
stdin is already connected to something above 0, then close the fd"
may be tricky to reason about.  There are four cases.

 (1) The first xopen(/dev/null, O_RDONLY) fails and returns -1.
     In this case, we go down the original codepath.

 (2) It returns 0 (upon startup, stdin was closed).  We keep the fd
     to use as the standard input, and go down the original
     codepath, but now standard input is connected, the original
     codepath needs to deal with only stdout and stderr, so we can
     safely use O_WRONLY and dup it into stdout and stderr.

 (3) It returns 1 or 2 (upon startup, stdin was open, stdin or
     stderr was closed).  The fd is closed, and we go to the
     original codepath.  Again, the original codepath does not have
     to touch stdin, so we can afford to use O_WRONLY.

 (4) It returns 3 or above, indicating that the three fd's we care
     about was already open.  We return without making further
     damage after closing the fd.

The updated code does a single system call to probe for the lowest
unused file descriptor and returns in the common case.  And that
single system call is an extra overhead for the "oops some of the
low FD were not open" case compared to the original code, but the
read-only FD may get reused when we need to.

So, I think the updated code does what it explains in its proposed
log message.  I.e.

    Check the need of sanitization with a file descriptor in read-only mode,
    keep it as replacement for stdin and open replacement file descriptors
    for stdout and stderr in write-only mode.

    v2:
      - switch to xopen("/dev/null", O_RDONLY) to stay at 2 syscalls in the
        common case and use O_WRONLY for stdout and stderr, as suggested
        by René Scharfe