Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.debian.bugs.dist > #1062916 > unrolled thread

Bug#990265: mksh: output error is not checked for some builtins

Back to article view | Back to linux.debian.bugs.dist

Contents

  Bug#990265: mksh: output error is not checked for some builtins Vincent Lefevre <vincent@vinc17.net> - 2021-06-24 12:00 +0200
    Bug#990264: Bug#990265: mksh: output error is not checked for some builtins when stdout is closed Thorsten Glaser <tg@mirbsd.de> - 2021-06-24 16:00 +0200
      Bug#990264: Bug#990265: mksh: output error is not checked for some builtins when stdout is closed Vincent Lefevre <vincent@vinc17.net> - 2021-06-24 16:30 +0200
        Bug#990264: Bug#990265: mksh: output error is not checked for some builtins when stdout is closed Vincent Lefevre <vincent@vinc17.net> - 2021-06-24 16:40 +0200
        Bug#990264: Bug#990265: mksh: output error is not checked for some builtins when stdout is closed Thorsten Glaser <tg@mirbsd.de> - 2021-06-24 17:20 +0200
      Bug#990264: Bug#990265: mksh: output error is not checked for some builtins when stdout is closed Anuradha Weeraman <anuradha@debian.org> - 2021-06-27 18:20 +0200
        Bug#990264: Bug#990265: Bug#990264: Bug#990265: mksh: output error is not checked for some builtins when stdout is closed Thorsten Glaser <tg@mirbsd.de> - 2021-06-27 19:40 +0200
        Bug#990264: Bug#990265: mksh: output error is not checked for some builtins when stdout is closed Vincent Lefevre <vincent@vinc17.net> - 2021-06-27 23:30 +0200
          Bug#990264: Bug#990265: mksh: output error is not checked for some builtins when stdout is closed Thorsten Glaser <tg@mirbsd.de> - 2021-06-27 23:50 +0200
    Bug#990265: utilities and write errors mirabilos <tg+ml@mirbsd.org> - 2021-06-30 03:00 +0200
      Bug#990265: utilities and write errors Vincent Lefevre <vincent-opgr@vinc17.net> - 2021-06-30 03:30 +0200
        Bug#990265: utilities and write errors Thorsten Glaser <tg@mirbsd.de> - 2021-06-30 04:10 +0200

#1062916 — Bug#990265: mksh: output error is not checked for some builtins

Package: mksh
Version: 59c-8
Severity: normal

Like with ksh93, output error is not checked for some builtins
(at least pwd and ulimit), e.g. after closing stdout.

$ exec >&-
$ for i in echo pwd print ulimit; do echo "[$i]" >&2; $i; echo $? >&2; done
[echo]
1
[pwd]

0
[print]
1
[ulimit]

0

Note also that for pwd, POSIX[*] says:

  If an error is detected, output shall not be written to standard
  output, a diagnostic message shall be written to standard error,
  and the exit status is not zero.

so that one would expect a diagnostic message (and I think that
one should also get one for the other builtins).

[*] https://pubs.opengroup.org/onlinepubs/9699919799/utilities/pwd.html

Note: Debian bug for ksh93:

  https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=990264

-- System Information:
Debian Release: 11.0
  APT prefers unstable-debug
  APT policy: (500, 'unstable-debug'), (500, 'testing-security'), (500, 'stable-updates'), (500, 'unstable'), (500, 'testing'), (500, 'stable'), (1, 'experimental')
Architecture: amd64 (x86_64)

Kernel: Linux 5.10.0-7-amd64 (SMP w/8 CPU threads)
Kernel taint flags: TAINT_PROPRIETARY_MODULE, TAINT_OOT_MODULE, TAINT_UNSIGNED_MODULE
Locale: LANG=POSIX, LC_CTYPE=C.UTF-8 (charmap=UTF-8), LANGUAGE not set
Shell: /bin/sh linked to /bin/dash
Init: systemd (via /run/systemd/system)
LSM: AppArmor: enabled

Versions of packages mksh depends on:
ii  libc6  2.31-12

Versions of packages mksh recommends:
ii  ed  1.17-1

mksh suggests no packages.

-- no debconf information

-- 
Vincent Lefèvre <vincent@vinc17.net> - Web: <https://www.vinc17.net/>
100% accessible validated (X)HTML - Blog: <https://www.vinc17.net/blog/>
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)

[toc] | [next] | [standalone]

#1062944 — Bug#990264: Bug#990265: mksh: output error is not checked for some builtins when stdout is closed

retitle 990265 mksh: output error is not checked for some builtins when stdout is closed
# notabug
close 990265
thanks

Vincent Lefevre dixit:

>Like with ksh93, output error is not checked for some builtins
>(at least pwd and ulimit), e.g. after closing stdout.
>
>$ exec >&-
>$ for i in echo pwd print ulimit; do echo "[$i]" >&2; $i; echo $? >&2; done

You have just left POSIX land:

   At program start-up, three streams are predefined and need not be
   opened explicitly: standard input (for reading conventional input),
   standard output (for writing conventional output), and standard error
   (for writing diagnostic output). When opened, the standard error
   stream is not fully buffered; the standard input and standard output
   streams are fully buffered if and only if the stream can be
   determined not to refer to an interactive device.

At the start of those commands, stdout is closed, so you violate that
constraint. Note POSIX is not just demands on the implementation but
also on the user, for example, it specifically permits extensions where
the behaviour is otherwise unspecified, i.e. which could not happen in
a conforming user.

Compare the second paragraph of https://mywiki.wooledge.org/FileDescriptor
and we’ve had a number of issues over the years, first reported by Jerker
Bäck, with closed stdout and stderr in various utilities finding that this
is not support{ed,able} at all.

>Note also that for pwd, POSIX[*] says:
>
>  If an error is detected, output shall not be written to standard

Pretty sure closed stdout is not an what they had in mind for that
utility.

>Note: Debian bug for ksh93:

Cc’ing; I can’t decide for aweeraman, but this is not a bug in mksh.

bye,
//mirabilos
-- 
Gestern Nacht ist mein IRC-Netzwerk explodiert. Ich hatte nicht damit
gerechnet, darum bin ich blutverschmiert… wer konnte ahnen, daß SIE so
reagier’n… gestern Nacht ist mein IRC-Netzwerk explodiert~~~
	(as of 2021-06-15 The MirOS Project temporarily reconvenes on OFTC)

[toc] | [prev] | [next] | [standalone]

#1062947 — Bug#990264: Bug#990265: mksh: output error is not checked for some builtins when stdout is closed

retitle 990265 mksh: output error is not checked for some builtins
reopen 990265
thanks

On 2021-06-24 13:49:55 +0000, Thorsten Glaser wrote:
> At the start of those commands, stdout is closed, so you violate that
> constraint. Note POSIX is not just demands on the implementation but
> also on the user, for example, it specifically permits extensions where
> the behaviour is otherwise unspecified, i.e. which could not happen in
> a conforming user.
[...]

Note that here, pwd is a builtin, so that there is no
"program start-up". So no constraints are violated.

Anyway, this was just an example. So, instead of a closed stdout,
let's write to a full file system:

$ pwd > /dev/full
$ echo $?

0
$ /bin/pwd > /dev/full
/bin/pwd: write error: No space left on device
$ echo $?
1

With the mksh builtin, the error is not detected.

Whatever POSIX says, it is important to report an error in such
a case, at least for the robustness of scripts.

Same problem with ksh93.

-- 
Vincent Lefèvre <vincent@vinc17.net> - Web: <https://www.vinc17.net/>
100% accessible validated (X)HTML - Blog: <https://www.vinc17.net/blog/>
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)

[toc] | [prev] | [next] | [standalone]

#1062948 — Bug#990264: Bug#990265: mksh: output error is not checked for some builtins when stdout is closed

retitle 990265 mksh: output error is not checked for some builtins
reopen 990265
thanks

On 2021-06-24 16:26:24 +0200, Vincent Lefevre wrote:
> Anyway, this was just an example. So, instead of a closed stdout,
> let's write to a full file system:
> 
> $ pwd > /dev/full
> $ echo $?
> 0
> $ /bin/pwd > /dev/full
> /bin/pwd: write error: No space left on device
> $ echo $?
> 1

Another example (due to the "sleep 1", the "true" has normally
terminated when pwd is executed):

$ { trap '' PIPE; sleep 1; pwd; echo $? >&2; } | true

0

both with mksh and ksh93.

With dash:

$ { trap '' PIPE; sleep 1; pwd; echo $? >&2; } | true
sh: 1: pwd: pwd: I/O error
1

-- 
Vincent Lefèvre <vincent@vinc17.net> - Web: <https://www.vinc17.net/>
100% accessible validated (X)HTML - Blog: <https://www.vinc17.net/blog/>
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)

[toc] | [prev] | [next] | [standalone]

#1062950 — Bug#990264: Bug#990265: mksh: output error is not checked for some builtins when stdout is closed

close 990265
thanks

Vincent Lefevre dixit:

>Note that here, pwd is a builtin, so that there is no
>"program start-up". So no constraints are violated.

This is wrong. pwd is allowed to be a builtin or not, and
POSIX is pretty specific in that this shall not make a difference.

>Anyway, this was just an example. So, instead of a closed stdout,
>let's write to a full file system:

These are all user errors.

>Whatever POSIX says, it is important to report an error in such
>a case, at least for the robustness of scripts.

Perhaps. But unless required by POSIX, I’m not going to do the
work. Utilities can, in general, assume that the standard I/O
streams work properly or it is either a user error or one they
cannot do much about anyway, and not catching errors on write(2),
printf(3), etc. is s̲o̲ common in the Unix world you’ll have to
live with it. In almost all cases, there’s also nothing one can
do about it.

For pwd(1) especially, I can argue that failure to write the
result is not actually an error. It determined the directory
properly and sent the information out. I’d argue that making
failure to write into an error for pwd(1) is a bug.

bye,
//mirabilos
-- 
Gestern Nacht ist mein IRC-Netzwerk explodiert. Ich hatte nicht damit
gerechnet, darum bin ich blutverschmiert… wer konnte ahnen, daß SIE so
reagier’n… gestern Nacht ist mein IRC-Netzwerk explodiert~~~
	(as of 2021-06-15 The MirOS Project temporarily reconvenes on OFTC)

[toc] | [prev] | [next] | [standalone]

#1063144 — Bug#990264: Bug#990265: mksh: output error is not checked for some builtins when stdout is closed

close 990264
thanks

On Thu, Jun 24, 2021 at 01:49:55PM +0000, Thorsten Glaser wrote:
> Vincent Lefevre dixit:
> 
> >Like with ksh93, output error is not checked for some builtins
> >(at least pwd and ulimit), e.g. after closing stdout.
> >
> >$ exec >&-
> >$ for i in echo pwd print ulimit; do echo "[$i]" >&2; $i; echo $? >&2; done
> 
> You have just left POSIX land:
> 
>    At program start-up, three streams are predefined and need not be
>    opened explicitly: standard input (for reading conventional input),
>    standard output (for writing conventional output), and standard error
>    (for writing diagnostic output). When opened, the standard error
>    stream is not fully buffered; the standard input and standard output
>    streams are fully buffered if and only if the stream can be
>    determined not to refer to an interactive device.
> 
> At the start of those commands, stdout is closed, so you violate that
> constraint. Note POSIX is not just demands on the implementation but
> also on the user, for example, it specifically permits extensions where
> the behaviour is otherwise unspecified, i.e. which could not happen in
> a conforming user.

There was an update on this from the Austin Group that will be in the
next version of the POSIX standard relating to the streams that need
to be opened at startup. Here's the changed verbiage which is explicit
about the streams needing to be "already open":

"At program start-up, three streams shall be predefined and already open:
stdin (standard input, for conventional input) for reading, stdout
(standard output, for conventional output) for writing, and stderr
(standard error, for diagnostic output) for writing." [1]

Also, some more elaboration by POSIX on unspecified behavior if the
standard streams are not open for reading and writing:

"If a standard utility or a conforming application is executed with file
descriptor 0 not open for reading or with file descriptor 1 or 2 not
open for writing, the environment in which the utility or application
is executed shall be deemed non-conforming, and consequently the utility
or application might not behave as described in this standard." [2]

[1] - https://austingroupbugs.net/view.php?id=1347#c5161
[2] - https://pubs.opengroup.org/onlinepubs/9699919799/functions/execve.html

-- 
Anuradha

[toc] | [prev] | [next] | [standalone]

#1063149 — Bug#990264: Bug#990265: Bug#990264: Bug#990265: mksh: output error is not checked for some builtins when stdout is closed

Anuradha Weeraman dixit:

>"At program start-up, three streams shall be predefined and already open:
>stdin (standard input, for conventional input) for reading, stdout
>(standard output, for conventional output) for writing, and stderr
>(standard error, for diagnostic output) for writing." [1]

Isn’t this what I already found in the online docs?

>[1] - https://austingroupbugs.net/view.php?id=1347#c5161

Ah, a clarification then. Very good! Thanks for spotting this.

>Also, some more elaboration by POSIX on unspecified behavior if the
>standard streams are not open for reading and writing:
>
>"If a standard utility or a conforming application is executed with file
>descriptor 0 not open for reading or with file descriptor 1 or 2 not
>open for writing, the environment in which the utility or application
>is executed shall be deemed non-conforming, and consequently the utility
>or application might not behave as described in this standard." [2]

Ah, good to know; this explicitly confirms that this is not a bug.
I should have looked there.

bye,
//mirabilos
-- 
Gestern Nacht ist mein IRC-Netzwerk explodiert. Ich hatte nicht damit
gerechnet, darum bin ich blutverschmiert… wer konnte ahnen, daß SIE so
reagier’n… gestern Nacht ist mein IRC-Netzwerk explodiert~~~
	(as of 2021-06-15 The MirOS Project temporarily reconvenes on OFTC)

[toc] | [prev] | [next] | [standalone]

#1063170 — Bug#990264: Bug#990265: mksh: output error is not checked for some builtins when stdout is closed

reopen 990264
reopen 990265
thanks

because the result of "pwd > /dev/full" is non POSIX conforming.

See the message from Don Cragun in the Austin Group list, which says:

Section 1.4 of the Commands and Utilities Volume of the standard
(Utility Description Defaults) already does this in the CONSEQUENCES
OF ERRORS section on P2303-2304, L74122-74151 in P1003.1-202x Draft 2
and on P2341-2342, L74540-74569 in P1003.1-2017.  Note especially:
    The following shall apply to each utility, unless
    otherwise stated:
        ... ... ...
        • When an unrecoverable error condition is
          encountered, the utility shall exit with a
          non-zero exit status.
        • A diagnostic message shall be written to
          standard error whenever an error condition
          occurs.

On 2021-06-27 21:33:48 +0530, Anuradha Weeraman wrote:
> There was an update on this from the Austin Group that will be in the
> next version of the POSIX standard relating to the streams that need
> to be opened at startup.
[...]

OK, but as said, that was just an example. There are other cases of
write errors, which correspond to valid conditions, such as when the
filesystem is full (see above).

-- 
Vincent Lefèvre <vincent@vinc17.net> - Web: <https://www.vinc17.net/>
100% accessible validated (X)HTML - Blog: <https://www.vinc17.net/blog/>
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)

[toc] | [prev] | [next] | [standalone]

#1063172 — Bug#990264: Bug#990265: mksh: output error is not checked for some builtins when stdout is closed

tags 990265 + moreinfo
outlook 990265 let's fight this out on the Austin mailing list
thanks

Vincent Lefevre dixit:

>because the result of "pwd > /dev/full" is non POSIX conforming.

I'm still not convinced, but let's take this elsewhere then until
we have some definite statements us shell maintainers can actually
comprehend and follow.

>OK, but as said, that was just an example. There are other cases of

If you wish to do me a favour, collect all of them, so I can fix
them if necessary, as I really do not have the tuits to do so.

Thanks,
//mirabilos

[toc] | [prev] | [next] | [standalone]

#1063344 — Bug#990265: utilities and write errors

Robert Elz via austin-group-l at The Open Group dixit:

>  | > You might prefer that it "fflush(stdout); if (ferror(stdout)) ..." but
>  | > there's nothing explicit in the standard that says that it has to do that.

>There was no argument based upon C functions - the use of them was
>just a shorthand to make a point.

Using the same shorthand, I changed mksh tonight to do pretty much
what you described, in a second patch:

@@ pseudo @@
 int
 call_builtin(func_t builtinfunc, int argc, char *argv[])
 {
     int exitstatus;

     stdout = fdopen(1, "w");
     exitstatus = (*builtinfunc)(argc, argv, environ);
     fflush(stdout);
+    if (ferror(stdout)) {
+        fprintf(stderr, "%s: write: %s\n", argv[0], strerror(errno));
+        if (exitstatus == 0)
+            exitstatus = 1;
+    }
     return (exitstatus);
 }

This handles pwd and (if compiled in) printf; I also added the same
error message to the echo/print builtin for where it detects errors
as it uses the write(2) syscall, not the stdio replacement, internally.

We’ll see whether that breaks anything. Incidentally, if true encounters
an error on stdout during operation now, it can return false — except it
never accesses stdout, thus this cannot happen.

Vincent will probably see whether this solves his problem; he’s got to
notify me if there are any other builtins to take care of.

Ugh. This is a “magic fix”… I don’t like this, but it’ll do, I think.
If the builtin encountered other errors and already exited nōn-zero,
that will be that, but otherwise, it runs on until the buffers are
flushed as write errors may not be recognised before that. If that’s
not enough, I don’t know. In that case I’d question the standard harder.

> exit() as part of its processing flushes the stdio buffers.  Including stdout.
> That's when the actual write from the process happens - but it is far too
> late then for pwd to do anything about it, exit() never returns, no pwd
> supplied code ever runs again (unless it set an atexit() - but it has no
> need for that, and doesn't).   I see nothing in the standard allowing exit()
> to terminate the process with any exit status other than the one handed to
> it, do you?   What's more, it would be extremely unlikely for exit() to be
> writing messages to stderr, which as you mentioned in a previous message is
> a requirement if the exit code is not 0 (for most utilities).

And yet, this is pretty much what I had to implement to hopefully
satisfy demands from here.

>  | The standard requires that pwd writes to stdout.
>
>The question is just what that means in terms of the interfaces available.
>pwd called printf, printf reported no error.   Stdout has been written to
>as far as I'm concerned when that has happened.

EXACTLY!

However, there’s one case in which I can see the reason of those
requesting this.

To set this up, I mounted a very small memory filesystem on /mnt,
made it owned by my user and created /mnt/foo, 0 bytes long, then
filled it up by writing to /mnt/bar etc. until no writes succeed.

Now, first the old, then the CVS HEAD shell:

tg@tglase-bsd:~ $ /bin/mksh -c pwd >/mnt/foo; echo $?

/mnt: write failed, filesystem is full

0
tg@tglase-bsd:~ $ /usr/obj/bin/mksh/mksh -c pwd >/mnt/foo; echo $?

/mnt: write failed, filesystem is full
/usr/obj/bin/mksh/mksh: pwd: write: No space left on device
1

The “write failed, filesystem is full” message comes from the kernel,
which is very helpful here. But yes, there’s the output. In scripts,
I’d expect writing “x=$(pwd)” instead of “pwd >x; x=$(cat x)”, but the
latter is necessary in some cases (e.g. escaping subshells, pipelines,
etc). In those situations, it would be nice if “pwd >x” failed, even
if it’s no problem of pwd(1) strictly spoken.

That’s my nightly change’s reason.

Tests (including bugreports, but let’s hope for none) welcome.

bye,
//mirabilos
-- 
Gestern Nacht ist mein IRC-Netzwerk explodiert. Ich hatte nicht damit
gerechnet, darum bin ich blutverschmiert… wer konnte ahnen, daß SIE so
reagier’n… gestern Nacht ist mein IRC-Netzwerk explodiert~~~
	(as of 2021-06-15 The MirOS Project temporarily reconvenes on OFTC)

[toc] | [prev] | [next] | [standalone]

#1063345 — Bug#990265: utilities and write errors

On 2021-06-30 00:46:00 +0000, mirabilos via austin-group-l at The Open Group wrote:
> Using the same shorthand, I changed mksh tonight to do pretty much
> what you described, in a second patch:
> 
> @@ pseudo @@
>  int
>  call_builtin(func_t builtinfunc, int argc, char *argv[])
>  {
>      int exitstatus;
> 
>      stdout = fdopen(1, "w");
>      exitstatus = (*builtinfunc)(argc, argv, environ);
>      fflush(stdout);
> +    if (ferror(stdout)) {
> +        fprintf(stderr, "%s: write: %s\n", argv[0], strerror(errno));
> +        if (exitstatus == 0)
> +            exitstatus = 1;
> +    }
>      return (exitstatus);
>  }
> 
> This handles pwd and (if compiled in) printf; I also added the same
> error message to the echo/print builtin for where it detects errors
> as it uses the write(2) syscall, not the stdio replacement, internally.

Thanks, I think that this is OK.

> We’ll see whether that breaks anything. Incidentally, if true encounters
> an error on stdout during operation now, it can return false — except it
> never accesses stdout, thus this cannot happen.
> 
> Vincent will probably see whether this solves his problem; he’s got to
> notify me if there are any other builtins to take care of.

There are "ulimit", "alias" and "type". But I don't know whether they
are also handled by call_builtin...

> Ugh. This is a “magic fix”… I don’t like this, but it’ll do, I think.
> If the builtin encountered other errors and already exited nōn-zero,
> that will be that, but otherwise, it runs on until the buffers are
> flushed as write errors may not be recognised before that. If that’s
> not enough, I don’t know. In that case I’d question the standard harder.

It seems that Coreutils has the same logic: the check is done only
at the end (except that it closes the stream instead of just flushing
it... which is OK because these are external commands).

-- 
Vincent Lefèvre <vincent@vinc17.net> - Web: <https://www.vinc17.net/>
100% accessible validated (X)HTML - Blog: <https://www.vinc17.net/blog/>
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)

[toc] | [prev] | [next] | [standalone]

#1063346 — Bug#990265: utilities and write errors

tags 990265 = pending
thanks

Vincent Lefevre dixit:

>> Vincent will probably see whether this solves his problem; he’s got to
>> notify me if there are any other builtins to take care of.
>
>There are "ulimit", "alias" and "type". But I don't know whether they
>are also handled by call_builtin...

All builtins are (including “direct builtin calls”, that is, executing
mksh with argv[0] set to the name of a builtin, like in beastiebox).

>Thanks, I think that this is OK.

Thanks; I’ll close this bug with a corresponding upload then, and if
you find anything new open another.

bye,
//mirabilos
-- 
Support mksh as /bin/sh and RoQA dash NOW!
‣ src:bash (398 (424) bugs: 0 RC, 266 (285) I&N, 132 (139) M&W, 0 F&P)
‣ src:dash (92 (107) bugs: 0 RC, 52 (56) I&N, 40 (51) M&W, 0 F&P)
‣ src:mksh (2 bugs: 0 RC, 0 I&N, 1 M&W, 1 F&P)
dash has two RC bugs they just closed because they don’t care about quality…

[toc] | [prev] | [standalone]

Back to top | Article view | linux.debian.bugs.dist

csiph-web