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:

NetBSD Home
NetBSD PR Database Search

(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.