GNU bug report logs -
#33965
handling of closed file descriptors
Previous Next
To reply to this bug, email your comments to 33965 AT debbugs.gnu.org.
Toggle the display of automated, internal messages from the tracker.
Report forwarded
to
bug-diffutils <at> gnu.org:
bug#33965; Package
diffutils.
(Thu, 03 Jan 2019 19:43:01 GMT)
Full text and
rfc822 format available.
Acknowledgement sent
to
Bruno Haible <bruno <at> clisp.org>:
New bug report received and forwarded. Copy sent to
bug-diffutils <at> gnu.org.
(Thu, 03 Jan 2019 19:43:04 GMT)
Full text and
rfc822 format available.
Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
There is a problem with the handling of closed file descriptors
in diff-3.7: As mentioned in [1], the 'new-file' test fails on HP-UX.
An investigation [2] shows that different coding techniques are needed,
depending on the desired outcome for closed file descriptors.
This patch uses a variant of (A) from [2]. Namely, the result of fcntl
can apparently be used to distinguish the substitute file descriptor
stuffed in by exec() from a regular open("/dev/null",O_RDONLY).
[1] https://lists.gnu.org/archive/html/diffutils-devel/2018-12/msg00019.html
[2] https://lists.gnu.org/archive/html/bug-gnulib/2019-01/msg00012.html
[0001-Recognize-file-descriptors-closed-by-the-parent-proc.patch (text/x-patch, attachment)]
Information forwarded
to
bug-diffutils <at> gnu.org:
bug#33965; Package
diffutils.
(Thu, 03 Jan 2019 20:03:02 GMT)
Full text and
rfc822 format available.
Message #8 received at submit <at> debbugs.gnu.org (full text, mbox):
I wrote:
> the result of fcntl
> can apparently be used to distinguish the substitute file descriptor
> stuffed in by exec() from a regular open("/dev/null",O_RDONLY).
Note that the code works for STDIN_FILENO, where fcntl(...) is 5 in one case
and 0 in the other case, and we use FD_CLOEXEC = 1 to distinguish the two.
For STDOUT_FILENO, fcntl(...) would be 5 in one case and 1 in the other case,
and the flag which allows to distinguish the two cases (4) is not listed in
<sys/fcntl.h>, therefore likely is a kernel-private flag.
Bruno
Reply sent
to
Jim Meyering <jim <at> meyering.net>:
You have taken responsibility.
(Fri, 04 Jan 2019 06:16:02 GMT)
Full text and
rfc822 format available.
Notification sent
to
Bruno Haible <bruno <at> clisp.org>:
bug acknowledged by developer.
(Fri, 04 Jan 2019 06:16:02 GMT)
Full text and
rfc822 format available.
Message #13 received at 33965-done <at> debbugs.gnu.org (full text, mbox):
On Thu, Jan 3, 2019 at 11:43 AM Bruno Haible <bruno <at> clisp.org> wrote:
>
> There is a problem with the handling of closed file descriptors
> in diff-3.7: As mentioned in [1], the 'new-file' test fails on HP-UX.
> An investigation [2] shows that different coding techniques are needed,
> depending on the desired outcome for closed file descriptors.
>
> This patch uses a variant of (A) from [2]. Namely, the result of fcntl
> can apparently be used to distinguish the substitute file descriptor
> stuffed in by exec() from a regular open("/dev/null",O_RDONLY).
>
> [1] https://lists.gnu.org/archive/html/diffutils-devel/2018-12/msg00019.html
> [2] https://lists.gnu.org/archive/html/bug-gnulib/2019-01/msg00012.html
Thank you. I have just pushed that with one tweak to the log message:
s/parent parent/parent/.
Information forwarded
to
bug-diffutils <at> gnu.org:
bug#33965; Package
diffutils.
(Fri, 04 Jan 2019 22:43:02 GMT)
Full text and
rfc822 format available.
Message #16 received at 33965 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
I'd rather think of this as a bug in the test case, not a bug in 'diff'.
This is because the test case assumes more about the shell's "<&-"
construct than what POSIX requires.
The bigger picture here is that invoking programs with stdin closed is
trouble, and although Gnulib provides several modules to work around the
trouble these modules are a hassle and many applications don't use them.
We should encourage platforms like HP-UX that try to work around the
trouble, instead of having diffutils attempt to simulate the traditional
but inferior exec semantics. Although the Linux kernel currently
implements the traditional semantics, in this particular case I think
I'd rather have diffutils adjust to what the platform provides, rather
than attempt to simulate Linux behavior even on non-Linux kernels.
So I propose the attached further patch.
[0001-diff-move-HP-UX-workaround-into-test-case.txt (text/plain, attachment)]
Did not alter fixed versions and reopened.
Request was from
Debbugs Internal Request <help-debbugs <at> gnu.org>
to
internal_control <at> debbugs.gnu.org.
(Fri, 04 Jan 2019 22:52:01 GMT)
Full text and
rfc822 format available.
Information forwarded
to
bug-diffutils <at> gnu.org:
bug#33965; Package
diffutils.
(Fri, 04 Jan 2019 23:31:02 GMT)
Full text and
rfc822 format available.
Message #21 received at 33965 <at> debbugs.gnu.org (full text, mbox):
Sorry, Paul, but I have to vehemently disagree here. You are mixing
two different questions:
(A) What is the best behaviour for a kernel?
(B) What is the best behaviour for a program?
About (A):
> We should encourage platforms like HP-UX that try to work around the
> trouble
The question is: Is it better for a kernel to be minimalist, hence
exec() does nothing special with the file descriptors? Or is it better
for a kernel to avoid pitfalls for programs, by protecting the first
3 file descriptors?
It is similar to the question: In the case of a signal delivery, should
a read() or write() system call return with a partial result or EINTR, or
should the kernel protect the programs from this knowledge and programming
overhead? This question was extensively discussed under the title
"worse is better", see
https://www.jwz.org/doc/worse-is-better.html
https://en.wikipedia.org/wiki/Worse_is_better
I have no personal opinion on this question.
About (B):
> I'd rather think of this as a bug in the test case, not a bug in 'diff'.
Here I disagree.
* The majority of the uses of 'diff' and coreutils is through shell scripts.
* <&- and >&- are POSIX-standardized syntaxes in shell scripts for a long
time.
* It is a goal to make the same shell script work the same way on different
platforms. The user gains nothing if you tell them "your shell script
works differently on Linux than on HP-UX because the kernel behaves
slightly differently".
* Therefore the gnulib modules
dirent-safer
fcntl-safer
fopen-safer
freopen-safer
openat-safer
pipe2-safer
popen-safer
stdlib-safer
tmpfile-safer
unistd-safer
serve a good purpose. They make programs running on Linux and
programs running on HP-UX behave more similarly, which is a good thing.
> This is because the test case assumes more about the shell's "<&-"
> construct than what POSIX requires.
It is like saying that it would be OK for a program, when interrupted
through Ctrl-Z and restarted through 'fg', to stop processing and exit
with a message "read: Interrupted system call". That's indeed how some
programs behaved around 1992: POSIX allowed the kernel to return from a
read() system call with errno = EINTR. But users don't want this. So it
was seen as a bug in the program, and fixed.
Same here.
Bruno
Information forwarded
to
bug-diffutils <at> gnu.org:
bug#33965; Package
diffutils.
(Sat, 05 Jan 2019 00:33:01 GMT)
Full text and
rfc822 format available.
Message #24 received at 33965 <at> debbugs.gnu.org (full text, mbox):
On 1/4/19 3:30 PM, Bruno Haible wrote:
>
>> the test case assumes more about the shell's "<&-"
>> construct than what POSIX requires.
> It is like saying that it would be OK for a program, when interrupted
> through Ctrl-Z and restarted through 'fg', to stop processing and exit
> with a message "read: Interrupted system call".
I don't see the analogy at all. Currently diffutils has a test shell
script that is not portable POSIX code. Although we've worked around the
problem with HP-UX by hacking on 'diff' itself, there are plausible
POSIX platforms where our current hack will not work. It's not
unreasonable to expect that test scripts should limit themselves to
POSIX features so that they are as portable as reasonably possible.
> POSIX allowed the kernel to return from a
> read() system call with errno = EINTR. But users don't want this. So it
> was seen as a bug in the program, and fixed.
If by "users" you mean "programs that make system calls", then many
users *do* want that. Certainly Emacs does, and I imagine many other
programs do too. If you meant something else by "users" then I'm not
sure I'm following the analogy.
For the stdin-is-closed situation, the users and developers of many (I
would say most) applications do not want 'open' to hijack stdin.
Although the HP-UX solution to this problem may not be best, it's better
than the traditional behavior as it is a simple change to the kernel
that fixes the issue for all applications, as opposed to the
more-complicated dances we do in openat-safer and friends.
After thinking about this a bit more, the message that I'm getting from
all this is that we should change diffutils to no longer rely on the
"feature" that stdin is closed, and remove the test cases that attempt
to test for this "feature". The feature is not portable to POSIX
platforms. Besides, I doubt whether anybody is using the feature anyway.
I put the feature in many years ago, so I can take the blame for
introducing the misguided feature and the responsibility for taking it out.
Information forwarded
to
bug-diffutils <at> gnu.org:
bug#33965; Package
diffutils.
(Sat, 05 Jan 2019 09:42:01 GMT)
Full text and
rfc822 format available.
Message #27 received at 33965 <at> debbugs.gnu.org (full text, mbox):
> > It is like saying that it would be OK for a program, when interrupted
> > through Ctrl-Z and restarted through 'fg', to stop processing and exit
> > with a message "read: Interrupted system call". ...
> > POSIX allowed the kernel to return from a
> > read() system call with errno = EINTR. But users don't want this. So it
> > was seen as a bug in the program, and fixed.
>
> If by "users" you mean "programs that make system calls"
By "users don't want this" I meant: Users don't want programs to terminate
with an error message "read: Interrupted system call", just because they
used the job control features of the shell.
Bruno
Information forwarded
to
bug-diffutils <at> gnu.org:
bug#33965; Package
diffutils.
(Sat, 05 Jan 2019 09:45:01 GMT)
Full text and
rfc822 format available.
Message #30 received at 33965 <at> debbugs.gnu.org (full text, mbox):
[CCing bug-gnulib. This is a followup to
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=33965 ]
Hi Paul,
> The feature is not portable to POSIX
> platforms. Besides, I doubt whether anybody is using the feature anyway.
OK, if you question the "feature" as a whole, then I'd say:
* Users don't *need* the "feature", in the sense that:
- The normal way of invoking a program is by providing arguments
and standard input/output streams. Users who want to invoke a program
are not actively looking for this contorted way to invoke a program.
- For most programs, equivalent functionality to the "feature" is available
by passing an empty string, a reference to an empty file, or /dev/null
as argument. If a program exists for which the equivalent functionality
is not available through the normal way of invocation, the developers
should better add it.
* But it's a security risk that a program can be invoked in ways in which it
was surely never tested. Anything can happen then - from overwriting precious
files of the user to endless loops.
* Therefore if you want to take out the "feature", you still have to think
about how to mitigate the security risks.
> Although the HP-UX solution to this problem may not be best, it's better
> than the traditional behavior as it is a simple change to the kernel
> that fixes the issue for all applications, as opposed to the
> more-complicated dances we do in openat-safer and friends.
Given
- the prevalent "worse is better" attitude,
- the "never break userland behaviour that programs may rely upon"
principle of the Linux kernel community,
- the fact that the glibc people looked at the problem and used the
HP-UX solution only for setuid programs,
I predict that this "simple change to the kernel" will not make it into
the majority of the operating systems in the next 10 years. (But you
could start lobbying for it among the OpenBSD people. They would be the
most likely to adopt it, I guess.)
In the mean time, the remaining options I can see are:
(a) Keep using the *-safer Gnulib modules.
(b) Introduce a sanity_check_file_descriptors() function that all programs
can invoke right at the beginning of main(), and that exits with an
exit code if it detects a closed fd.
void
sanity_check_file_descriptors (void)
{
int fd;
for (fd = 0; fd <= 2; fd++)
if (fcntl (fd, F_GETFD, NULL) < 0 && errno == EBADF)
exit (125);
}
On platforms like HP-UX, this code would not cause an exit, but
the exec() call has already provided substitutes for the closed fds;
this mitigates the security risks.
I would propose exit code 125, since 127 and 126 are already taken [1].
(b) surely is simpler than (a).
Bruno
[1] http://tldp.org/LDP/abs/html/exitcodes.html
Information forwarded
to
bug-diffutils <at> gnu.org:
bug#33965; Package
diffutils.
(Sun, 06 Jan 2019 04:08:02 GMT)
Full text and
rfc822 format available.
Message #33 received at 33965 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Bruno Haible wrote:
>> The feature is not portable to POSIX
>> platforms. Besides, I doubt whether anybody is using the feature anyway.
>
> OK, if you question the "feature" as a whole,
Sorry, I was referring only to the never-used feature of diffutils, where "-" in
a command-line argument stands for a missing file if stdin is closed (only in
some cases, e.g., -N). I think nobody uses this feature or will miss it if it's
removed, and anyway it can't work on some POSIX-conforming platforms. I put the
feature into diffutils in many years ago, but it's now clear that the test case
that is exercising it is a mistake and that the feature itself was mistaken.
But to broaden this to the more-general issue of whether kernels should force
stdin/stdout/stderr to be open after an exec:
> I predict that this "simple change to the kernel" will not make it into
> the majority of the operating systems in the next 10 years. (But you
> could start lobbying for it among the OpenBSD people. They would be the
> most likely to adopt it, I guess.)
OpenBSD has already adopted it for setuid and setgid executables, as has
FreeBSD, NetBSD, and SELinux. I imagine this feature is reasonably common
elsewhere, at least for setuid (I haven't checked carefully). As far as I know
only HP-UX does it for non-setuid/gid executables.
> In the mean time, the remaining options I can see are:
>
> (a) Keep using the *-safer Gnulib modules.
>
> (b) Introduce a sanity_check_file_descriptors() function
> ...
> void
> sanity_check_file_descriptors (void)
> {
> int fd;
> for (fd = 0; fd <= 2; fd++)
> if (fcntl (fd, F_GETFD, NULL) < 0 && errno == EBADF)
> exit (125);
> }
> ...
> (b) surely is simpler than (a).
I suggest instead a third option (c) that was formerly used in Coreutils, which
is that the program makes sure that stdin/stdout/stderr are open before doing
anything that creates a file descriptor. As I understand it Coreutils moved away
from that approach primarily because it was redundant if the (earlier) *-safer
modules are also used. However, in hindsight perhaps Coreutils should have
stopped using the *-safer modules and just used method (c); it's a lot simpler
and less intrusive.
I resurrected and refreshed that old Coreutils code and put it into a new Gnulib
module stdopen, adjusted Gnulib's xfreopen module in a couple of minor ways
(only diffutils uses xfreopen as far as I know) so that it no longer requires
freopen-safer, and installed the first four attached patches into Gnulib. I then
propagated this into diffutils and removed the unportable feature by installing
the last three attached patches into diffutils.
As a result of these changes, diffutils no longer uses the *-safer modules (it
uses the new stdopen module instead), it no longer supports the unportable
feature in question, and it no longer mishandles some obscure situations where
stdin, stdout, or stderr are closed.
[0001-stdopen-copy-from-last-use-in-coreutils.patch (text/x-patch, attachment)]
[0002-stdopen-modernize-and-simplify.patch (text/x-patch, attachment)]
[0003-xfreopen-need-not-depend-on-freopen-safer.patch (text/x-patch, attachment)]
[0004-xfreopen-need-not-include-stdio-.h.patch (text/x-patch, attachment)]
[0001-build-update-gnulib-submodule-to-latest.patch (text/x-patch, attachment)]
[0002-diff-remove-unportable-diff-N-f-feature.patch (text/x-patch, attachment)]
[0003-diff-fix-cmp-diff3-sdiff-with-stdin-closed.patch (text/x-patch, attachment)]
Information forwarded
to
bug-diffutils <at> gnu.org:
bug#33965; Package
diffutils.
(Mon, 07 Jan 2019 01:48:02 GMT)
Full text and
rfc822 format available.
Message #36 received at 33965 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Thanks for the xstdopen module; it simplifies diffutils and I installed the
attached patches there for it.
[0001-build-update-gnulib-submodule-to-latest.patch (text/x-patch, attachment)]
[0002-diff-use-xstdopen-not-stdopen.patch (text/x-patch, attachment)]
This bug report was last modified 6 years and 160 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.