NetBSD Problem Report #51603
From www@NetBSD.org Sat Nov 5 20:51:58 2016
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
(Client CN "mail.netbsd.org", Issuer "Postmaster NetBSD.org" (verified OK))
by mollari.NetBSD.org (Postfix) with ESMTPS id DCC737A279
for <gnats-bugs@gnats.NetBSD.org>; Sat, 5 Nov 2016 20:51:58 +0000 (UTC)
Message-Id: <20161105205157.7C84C7A2CC@mollari.NetBSD.org>
Date: Sat, 5 Nov 2016 20:51:57 +0000 (UTC)
From: n54@gmx.com
Reply-To: n54@gmx.com
To: gnats-bugs@NetBSD.org
Subject: WIFCONTINUED()==true always implies WIFSTOPPED()==true in the current implementation
X-Send-Pr-Version: www-1.0
>Number: 51603
>Category: standards
>Synopsis: WIFCONTINUED()==true always implies WIFSTOPPED()==true in the current implementation
>Confidential: no
>Severity: serious
>Priority: medium
>Responsible: standards-manager
>State: closed
>Class: sw-bug
>Submitter-Id: net
>Arrival-Date: Sat Nov 05 20:55:00 +0000 2016
>Closed-Date: Sun Nov 06 00:03:10 +0000 2016
>Last-Modified: Sun Nov 06 02:30:00 +0000 2016
>Originator: Kamil Rytarowski
>Release: NetBSD 7.99.42 amd64
>Organization:
TNF
>Environment:
NetBSD chieftec 7.99.42 NetBSD 7.99.42 (GENERIC) #0: Sat Nov 5 15:49:14 CET 2016 root@chieftec:/tmp/netbsd-tmp/sys/arch/amd64/compile/GENERIC amd64
>Description:
Current implementation of <sys/wait.h> implies that a status of wait(2)-like functions for WIFCONTINED()==true implies always true.
In the current code of <sys/wait.h>:
#if !( defined(_XOPEN_SOURCE) || defined(_NETBSD_SOURCE) ) || defined(_KERNEL)
#define _W_INT(i) (i)
#else
#define _W_INT(w) (*(int *)(void *)&(w)) /* convert union wait to int */
#endif
#define _WSTATUS(x) (_W_INT(x) & 0177)
#define _WSTOPPED 0177 /* _WSTATUS if process is stopped */
#define _WCONTINUED 0xffffU
#define WIFSTOPPED(x) (_WSTATUS(x) == _WSTOPPED)
#define WIFCONTINUED(x) (_W_INT(x) == _WCONTINUED)
#define WSTOPSIG(x) ((int)(((unsigned int)_W_INT(x)) >> 8) & 0xff)
#define WIFSIGNALED(x) (_WSTATUS(x) != _WSTOPPED && _WSTATUS(x) != 0)
#define WTERMSIG(x) (_WSTATUS(x))
#define WIFEXITED(x) (_WSTATUS(x) == 0)
#define WEXITSTATUS(x) ((int)(((unsigned int)_W_INT(x)) >> 8) & 0xff)
WIFSTOPPED() checks a bit submask of WIFCONTINUED()
>How-To-Repeat:
The code triggering this issue will be committed to tests/lib/libc/sys/t_wait.c once ID number will be assigned to this PR.
atf-run t_wait | atf-report
>Fix:
TBD
WIFCONTINUED() was added in April 2016 and is not widely used, therefore I'm fine with changing the code with breaking backwards compatibility.
>Release-Note:
>Audit-Trail:
From: "Kamil Rytarowski" <kamil@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/51603 CVS commit: src/tests/lib/libc/sys
Date: Sat, 5 Nov 2016 21:49:28 +0000
Module Name: src
Committed By: kamil
Date: Sat Nov 5 21:49:28 UTC 2016
Modified Files:
src/tests/lib/libc/sys: t_wait.c
Log Message:
Add new test wait6_stopgo_loop in t_wait
This test verifies that it is possible to emit multiple times SIGSTOP and
SIGCONT for a child.
Add checks that status does not return more than one valid state from the
following list: STOPPED, CONTINUED, EXITED and SIGNALED. This check fails
for WIFCONTINUED()==true as it currently and wrongly returns true for
WIFSTOPPED().
This verification is added to wait6_stopgo_loop and wait6_stop_and_go and
marked as expected failure and linked with PR standards/51603.
Remove trailing whitespace.
Sponsored by <The NetBSD Foundation>
To generate a diff of this commit:
cvs rdiff -u -r1.4 -r1.5 src/tests/lib/libc/sys/t_wait.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
From: Robert Elz <kre@munnari.OZ.AU>
To: gnats-bugs@NetBSD.org
Cc:
Subject: Re: standards/51603: WIFCONTINUED()==true always implies WIFSTOPPED()==true in the current implementation
Date: Sun, 06 Nov 2016 05:10:44 +0700
To add a little to Kamil's bug report...
POSIX requires that at exactly one of (the relevant) WIFxxxx() macros
be true (which are relevant depends upon which wait*() function is used,
and when possible, what flags are passed.)
As we currently have the values defined, WIFCONTINUED() == true
implies WIFSTOPPED() == true which POSIX does not permit.
kre
From: David Holland <dholland-bugs@netbsd.org>
To: gnats-bugs@NetBSD.org
Cc:
Subject: Re: standards/51603: WIFCONTINUED()==true always implies
WIFSTOPPED()==true in the current implementation
Date: Sat, 5 Nov 2016 22:26:30 +0000
On Sat, Nov 05, 2016 at 08:55:00PM +0000, n54@gmx.com wrote:
> Current implementation of <sys/wait.h> implies that a status of
> wait(2)-like functions for WIFCONTINED()==true implies always true.
I don't think that sentence says what you meant it to say :-)
> In the current code of <sys/wait.h>:
So the traditional encoding, dumb as it may be, is as follows:
0000 0000 0000 0000 BBBB BBBB CAAA AAAA
|- 31..16 -| |-15..8-| 7|-6..0-|
- if A is 0, it means EXITED and B is the exit status.
- if A is 0x7f, it means STOPPED and B is the stop signal.
- if A is some other value, SIGNALED and A is the exit signal.
- if C is 1 it means a core file was dumped.
It seems that when WIFCONTINUED was added it was meant to be indicated
by having the whole value equal to 0xffff. In the above encoding this
value indicates STOPPED on signal 255 with a core dump. This isn't
what we want.
ISTM that the best way to fix this is to add this rule after the rule
for STOPPED:
- if A is 0x7e, it means CONTINUED.
(since I guess there's no need to indicate a continue signal)
Since there aren't 126 signals, reserving another value will not break
the existing logic.
Naive code will then interpret the new WIFSTOPPED as SIGNALED with
signal 126 but naive code that didn't ask for them shouldn't be
getting WIFSTOPPED notices. With this change one won't get the wrong
answer by testing WIFSIGNALED before WIFSTOPPED, and that seems like
the important part.
However, if that's not good enough we need to use more bits in the
upper half of the word, because every possible combination of the
lower bits already means something.
One possible way to do this is to make 0x10000 a flag indicating
WIFSTOPPED, and then if that bit's 0 revert to the rest of the logic.
A second way is to make another field D that's the next 4 or 8 bits,
and use that to indicate what happened and then also set the other
fields according to the traditional encoding, for compatibility.
I don't think either of these is a good plan; this encoding is, though
historical, silly, and if we're going to change it we might as well
just change it, insert the compat code needed, and not have to think
about it again the next time the Linux world or the POSIX committee
thinks up another silly thing to overload wait() with.
Here's what I would propose as a new encoding:
- bits 0-7 indicate what happened
- bits 8-31 hold the accompanying value, if any.
Because POSIX insists that the exit status be truncated to 8 bits,
that would produce the following implementation:
#define _WSTATUS(x) (_W_INT(x) & 0xff)
#define _WVALUE(x) ((int)((unsigned)_W_INT(x) >> 8))
#define _WMAKE(v, s) ((((unsigned)(v)v) << 8) | (s))
#define _WEXITED 0
#define _WSIGNALED 1
#define _WCORED 2
#define _WSTOPPED 3
#define _WCONTINUED 4
/* add more as people make stuff up */
#define WIFEXITED(x) (_WSTATUS(x) == _WEXITED)
#define WIFSIGNALED(x) (_WSTATUS(x) == _WEXITED || _WSTATUS(x) == _WCORED)
#define WCOREDUMP(x) (_WSTATUS(x) == _WCORED)
#define WIFSTOPPED(x) (_WSTATUS(x) == _WSTOPPED)
#define WIFCONTINUED(x) (_WSTATUS(x) == _WCONTINUED)
#ifdef _POSIX_MISTAKE_SOURCE
#define WEXITSTATUS(x) (_WVALUE(x) & 0xff)
#else
#define WEXITSTATUS(x) _WVALUE(x)
#endif
#define WTERMSIG(x) _WVALUE(x)
#define WSTOPSIG(x) _WVALUE(x)
although the implementation of W_EXITCODE becomes messy:
#define W_EXITCODE(ret, sig) \
((sig)==0 ? _WMAKE(ret, _WEXITED) : _WAKE(sig, _WSIGNALED))
and also doesn't interact well with _WCORED, so it would be better to
replace it with
#define W_EXITEDCODE(ret) _WMAKE(ret, _WEXITED)
#define W_SIGNALEDCODE(sig) _WMAKE(sig, _WSIGNALED)
#define W_COREDCODE(sig) _WMAKE(sig, _WCORED)
#define W_STOPCODE(sig) _WMAKE(sig, _WSTOPPED)
#define W_CONTINUEDCODE() _WMAKE(0, _WCONTINUED)
but as (AFAIK) W_EXITCODE is not standard there's no big problem with
this.
> #if !( defined(_XOPEN_SOURCE) || defined(_NETBSD_SOURCE) ) || defined(_KERNEL)
> #define _W_INT(i) (i)
> #else
> #define _W_INT(w) (*(int *)(void *)&(w)) /* convert union wait to int */
> #endif
Unrelatedly, this should go away. Any code floating around still using
union wait needs to be patched already. union wait's been deprecated
for more than twenty years.
--
David A. Holland
dholland@netbsd.org
From: Robert Elz <kre@munnari.OZ.AU>
To: gnats-bugs@NetBSD.org
Cc:
Subject: Re: standards/51603: WIFCONTINUED()==true always implies WIFSTOPPED()==true in the current implementation
Date: Sun, 06 Nov 2016 06:52:35 +0700
Date: Sat, 5 Nov 2016 22:30:01 +0000 (UTC)
From: David Holland <dholland-bugs@netbsd.org>
Message-ID: <20161105223001.6675D7A285@mollari.NetBSD.org>
| It seems that when WIFCONTINUED was added it was meant to be indicated
| by having the whole value equal to 0xffff. In the above encoding this
| value indicates STOPPED on signal 255 with a core dump. This isn't
| what we want.
No, but it is also what linux does, and if handled correctly (the values
are only supposed to be tested using the WIFxxx() macros, not by manually
decoding the bits) it is possible to make it work that way.
| ISTM that the best way to fix this is to add this rule after the rule
| for STOPPED:
|
| - if A is 0x7e, it means CONTINUED.
That was my original suggestion (Kamil, Christos, and I have been discussing
this problem for the past few days - off list).
| Naive code will then interpret the new WIFSTOPPED as SIGNALED with
| signal 126 but naive code that didn't ask for them shouldn't be
| getting WIFSTOPPED notices. With this change one won't get the wrong
| answer by testing WIFSIGNALED before WIFSTOPPED, and that seems like
| the important part.
s/WIFSTOPPED/WIFCONTINUED/ in that paragraph for it to make any sense...
And yes, this way would work. Unfortunately, it also breaks existing
code (apps) that have been compiled opn -current in the past 6 months, and
which use WCONTINUED and WIFCONTINUED, as their tests for WIFCONTINUED
would never succeed after the fix.
I have no idea how much code there is like that - (shells/bash) is one.
There is no code in the NetBSD source tree that tests WIFCONTINUED()
so the problem applies only to external code (pkgsrc and other.)
There is, interestingly, some in-tree NetBSD side that sets WCONTINUED on
a wait call - I think based upon the "if the flag exists, we should use it".
The one example I am thinking of actually wants
waitpid(pid, &status, 0);
but does
waitpid(pid, &status, WUNTRACED|WCONTINUED);
and then proceeds to ignore any processes that are returned that are
not WIFEXITED() or WIFSIGNALLED(). Bizarre. [Aside: this is
externally imported code, not written by the NetBSD project, there is
no project code at all that references the CONTINUED stuff at all,
if we ignore the ATF tests which explicitly test it, and the kernel which
sets it, of course.)
| However, if that's not good enough we need to use more bits in the
| upper half of the word, because every possible combination of the
| lower bits already means something.
Actually, I think 0x80 doesn't - exited normally with a core dump is
a fairly insane combination, and 0xFF doesn't either, stopped with a
code dump is just as meaningless (we could change the test for WIFSTOPPED
to check that the low 8 bits are 0177 rather than just the low 7 bits as
has been traditionally done.)
| A second way is to make another field D that's the next 4 or 8 bits,
| and use that to indicate what happened and then also set the other
| fields according to the traditional encoding, for compatibility.
Anything like this also needs to consider what happens with waitid()
which has a 32 bit exit code.
| Because POSIX insists that the exit status be truncated to 8 bits,
except for waitid() (that is actually a bit less clear, but there are
significant arguments in the posix community that that is what was intended
when the exit status field was made 32 bits in the struct that waitid()
returns.)
It is possible to make a fix for this by just altering the WIFxxx() macros
(WIFSTOPPED can just add "&& !WIFcONTINUED(...)" for example, which
guarantees that they can't both be true together. That satisfies posix.
It also has the advantage that existing compiled code keeps on working
as well as it now does, which any change to the _WCONTINUED value does not.
Since we have had zero bugs reports about code not working, and since linux
also uses 0xFFFF as the _WCONTINUED value (FreeBSD does not, their
WIFCONTINUED() is "stopped with a SIGCONT" (and that case is excluded from
WIFSIGNALLED().)
kre
State-Changed-From-To: open->closed
State-Changed-By: kre@NetBSD.org
State-Changed-When: Sun, 06 Nov 2016 00:03:10 +0000
State-Changed-Why:
Fixed by Christos in src/sys/sys/wait.h 1.32
From: Kamil Rytarowski <n54@gmx.com>
To: gnats-bugs@NetBSD.org
Cc:
Subject: Re: standards/51603: WIFCONTINUED()==true always implies
WIFSTOPPED()==true in the current implementation
Date: Sun, 6 Nov 2016 00:57:03 +0100
This is an OpenPGP/MIME signed message (RFC 4880 and 3156)
--P1MkoRgtWINvNtDo8Pqf5x9qgm8roR11r
Content-Type: multipart/mixed; boundary="tMD0viw2ARAU6XwVf33NrN9KS7bF630rX"
From: Kamil Rytarowski <n54@gmx.com>
To: gnats-bugs@NetBSD.org
Message-ID: <0a07ec5d-4c6f-b23f-b9f1-30f88d903c8d@gmx.com>
Subject: Re: standards/51603: WIFCONTINUED()==true always implies
WIFSTOPPED()==true in the current implementation
References: <pr-standards-51603@gnats.netbsd.org>
<20161105205157.7C84C7A2CC@mollari.NetBSD.org>
<20161105235501.78FE17A2C7@mollari.NetBSD.org>
In-Reply-To: <20161105235501.78FE17A2C7@mollari.NetBSD.org>
--tMD0viw2ARAU6XwVf33NrN9KS7bF630rX
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: quoted-printable
On 06.11.2016 00:55, Robert Elz wrote:
> And yes, this way would work. Unfortunately, it also breaks existin=
g
> code (apps) that have been compiled opn -current in the past 6 months,=
and
> which use WCONTINUED and WIFCONTINUED, as their tests for WIFCONTINUED=
> would never succeed after the fix.
> =20
> I have no idea how much code there is like that - (shells/bash) is one=
=2E
> =20
Here is a list from Debian to make initial idea how much might be there:
glibc gnunet octave diod bash ace qgis golang icedove luxio passenger
krb5 ats2-lang clisp kfreebsd-10 qpxtool prelude-lml gammu moin tuxpaint
hhvm pacemaker health-check ruby-nio4r mksh network-manager-vpnc nsca-ng
tarantool-lts gridengine eric strace nspr audit cupt python-gevent
libproc-waitstat-perl firefox-esr cctools mypy bluefish radare2 ivykis
ksh zsh libconvert-binary-c-perl care libev-perl ocamlbricks criu musl
amule trinity rustc firefox gtk-gnutella geshi sbcl vzctl newlib
opensmtpd pygments libfixposix prelude-manager ruby-pygments.rb shedskin
network-manager-openvpn gparted deheader php7.0 uclibc kbuild stress-ng
blktap openjfx anytun bluez rxvt-unicode network-manager python3.5 gvpe
pypy nordugrid-arc-nagios-plugins yash sssd valgrind jython tarantool
qscintilla2 vala libev emscripten network-manager-applet cython
monkeystudio nim systemd slang2 amanda proot syslog-ng python2.7 acl2
zabbix tulip systemtap ldc ats-lang-anairiats gtksourceview3 libexplain
dokuwiki network-manager-pptp
https://codesearch.debian.net/search?q=3DWIFCONTINUED
Someone willing to prepare codesearch.pkgsrc.org?
--tMD0viw2ARAU6XwVf33NrN9KS7bF630rX--
--P1MkoRgtWINvNtDo8Pqf5x9qgm8roR11r
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: OpenPGP digital signature
Content-Disposition: attachment; filename="signature.asc"
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2
iQIcBAEBCAAGBQJYHnHaAAoJEEuzCOmwLnZs5UgP/2khIqnDAwAM71ruwH4UlF8z
EDmbkNlKU/B6IBXZWAQuD/YacdtOF412r0uOhx+VfJttlo7vwS7GSKJuUA0Pjj/L
xewTQlnKXN/G9UvDeQdQYIdMITY/yY0HgRyTNeKfkSpMy7zjT3IPDWDEKeFUq2X7
uw4EDJ6hjsoWCvMV8ovpez7GZLjZovNleRkfVv2IkkS+RUeQKnMVMBp2kTCyiOdu
+x0q1Act67+6vlOWHuXffK8cKWQExQZXr6y2LKv3RejdxoXW7Rn+aFsWy5WLCamp
mp8SXndlLjiKUhrZJ26TqTHiTyNzSNh7Uj6hkEcfiwzavSN+gV+GDOV4f4YRvFY4
Cdj7Qu1pO7fT28pJ5MPgni93hdk2UA4PSZ17DBxmxZFa8cJTLzzbJnTcFrUbfXrs
yJNIr57r4pkJG8bFx5DyzBeB9fCWGSOpLFbHsQhgIqBPN8BML8QtRSobyMU5i/UV
W/AIv0WOccTE7lbVbw/zYHdz0HCjzJZEFYXGCSpomxzHSKOtF5TeiOH1qdiFKh4L
HGSfoN5mLCJTD15Akx9bVKjuNL8zGZiVlVbmWuQ0CinWwlNL5xMvFoCMH7r3FnW+
171zZUxpQ3p4wghDFPwsRvUnQWdd2s4fdBwucZPCZtcNL8/aJoAWfDdzEB4S/zrH
LehqhpM2hHS/KsLQ5+u3
=zPn7
-----END PGP SIGNATURE-----
--P1MkoRgtWINvNtDo8Pqf5x9qgm8roR11r--
From: "Kamil Rytarowski" <kamil@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/51603 CVS commit: src/tests/lib/libc/sys
Date: Sun, 6 Nov 2016 02:28:57 +0000
Module Name: src
Committed By: kamil
Date: Sun Nov 6 02:28:57 UTC 2016
Modified Files:
src/tests/lib/libc/sys: t_wait.c
Log Message:
All tests in t_wait now pass
Christos Zoulas committed fixes to src/sys/sys/wait.h r.1.32.
Closes PR standards/51603
Sponsored by <The NetBSD Foundation>
To generate a diff of this commit:
cvs rdiff -u -r1.5 -r1.6 src/tests/lib/libc/sys/t_wait.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
>Unformatted:
(Contact us)
$NetBSD: query-full-pr,v 1.39 2013/11/01 18:47:49 spz Exp $
$NetBSD: gnats_config.sh,v 1.8 2006/05/07 09:23:38 tsutsui Exp $
Copyright © 1994-2014
The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.
Home