Bug 211399 - usr.sbin/etcupdate/preworld_test:main because of r303047
Summary: usr.sbin/etcupdate/preworld_test:main because of r303047
Status: Closed Overcome By Events
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Some People
Assignee: Enji Cooper
URL: https://reviews.freebsd.org/D7368
Keywords: regression
Depends on:
Blocks:
 
Reported: 2016-07-27 03:02 UTC by Enji Cooper
Modified: 2017-01-03 06:27 UTC (History)
3 users (show)

See Also:


Attachments
Test-case (49 bytes, application/x-shellscript)
2016-07-30 16:17 UTC, Mikhail T.
no flags Details
Fix and then some (2.73 KB, patch)
2016-07-30 18:26 UTC, Mikhail T.
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Enji Cooper freebsd_committer 2016-07-27 03:02:14 UTC
usr.sbin/etcupdate/preworld_test:main started failing on https://jenkins.freebsd.org/job/FreeBSD_HEAD/471/ . r303047 seems like the most likely culprit.

Repro:

Run `kyua debug -k /usr/tests/usr.sbin/etcupdate/Kyuafile preworld_test:main` as root, e.g.

$ sudo kyua debug -k /usr/tests/usr.sbin/etcupdate/Kyuafile preworld_test:main
Differences for -n:
Differences for real:
Checking tree for correct results:
File /etc/master.passwd has wrong contents
File /etc/group has wrong contents
File /etc/passwd should be a regular file
File /etc/pwd.db should be a regular file
File /etc/spwd.db should be a regular file
preworld_test:main  ->  failed: Returned non-success exit status 1
Comment 1 Enji Cooper freebsd_committer 2016-07-27 03:03:58 UTC
(In reply to Ngie Cooper from comment #0)

Confirmed -- r303047 is the culprit:

$ (set -e; cd /usr/src/svn/usr.bin/sed/; svn merge -c -r303047 .; make obj; make depend; make all; sudo make install)
...
$ sudo kyua debug -k /usr/tests/usr.sbin/etcupdate/Kyuafile preworld_test:main
Differences for -n:
Differences for real:
Checking tree for correct results:
preworld_test:main  ->  passed
Comment 2 Enji Cooper freebsd_committer 2016-07-27 03:10:28 UTC
sed's succeeding... it's a change in behavior somehow that wouldn't have been caught by the etcupdate tests if that had used `atf_check ... sed ...` instead of `sed ...`:

$ sudo kyua debug -k /usr/tests/usr.sbin/etcupdate/Kyuafile preworld_test:main
0
0
0
Differences for -n:
Differences for real:
Checking tree for correct results:
File /etc/master.passwd has wrong contents
File /etc/group has wrong contents
File /etc/passwd should be a regular file
File /etc/pwd.db should be a regular file
File /etc/spwd.db should be a regular file
preworld_test:main  ->  failed: Returned non-success exit status 1
Comment 3 Pedro F. Giffuni freebsd_committer 2016-07-27 03:26:25 UTC
Mikhail can you please take a look?

We can revert r303047 (and r303146) if needed.
Comment 4 Enji Cooper freebsd_committer 2016-07-27 03:36:28 UTC
-i vs -I isn't the issue...
Comment 5 Enji Cooper freebsd_committer 2016-07-27 06:09:53 UTC
Here's the difference:

root:<rpass>:0:0::0:0:Charlie &:/root:/bin/csh^M
toor:*:0:0::0:0:Bourne-again Superuser:/root:^M
daemon:*:1:1::0:0:Owner of many system processes:/root:/usr/sbin/nologin^M
operator:*:2:5::0:0:System &:/:/usr/sbin/nologin^M
_dhcp:*:65:65::0:0:dhcp programs:/var/empty:/usr/sbin/nologin^M
uucp:*:66:66::0:0:UUCP pseudo-user:/var/spool/uucppublic:/usr/local/libexec/uucp/uucico^M
pop:*:68:6::0:0:Post Office Owner:/nonexistent:/usr/sbin/nologin^M
auditdistd:*:78:77::0:0:Auditdistd unprivileged user:/var/empty:/usr/sbin/nologin^M
www:*:80:80::0:0:World Wide Web Owner:/nonexistent:/usr/sbin/nologin^M
hast:*:845:845::0:0:HAST unprivileged user:/var/empty:/usr/sbin/nologin^M
nobody:*:65534:65534::0:0:Unprivileged user:/nonexistent:/usr/sbin/nologin^M
john:<password>:1001:1001::0:0:John Baldwin:/home/john:/bin/tcsh^M
messagebus:*:556:556::0:0:D-BUS Daemon User:/nonexistent:/usr/sbin/nologin^M
polkit:*:562:562::0:0:PolicyKit User:/nonexistent:/usr/sbin/nologin^M
haldaemon:*:560:560::0:0:HAL Daemon User:/nonexistent:/usr/sbin/nologin^M

#^M
root:<rpass>:0:0::0:0:Charlie &:/root:/bin/csh^M
toor:*:0:0::0:0:Bourne-again Superuser:/root:^M
daemon:*:1:1::0:0:Owner of many system processes:/root:/usr/sbin/nologin^M
operator:*:2:5::0:0:System &:/:/usr/sbin/nologin^M
_dhcp:*:65:65::0:0:dhcp programs:/var/empty:/usr/sbin/nologin^M
uucp:*:66:66::0:0:UUCP pseudo-user:/var/spool/uucppublic:/usr/local/libexec/uucp/uucico^M
pop:*:68:6::0:0:Post Office Owner:/nonexistent:/usr/sbin/nologin^M
auditdistd:*:78:77::0:0:Auditdistd unprivileged user:/var/empty:/usr/sbin/nologinwww:*:80:80::0:0:World Wide Web Owner:/nonexistent:/usr/sbin/nologin^M
hast:*:845:845::0:0:HAST unprivileged user:/var/empty:/usr/sbin/nologin^M
nobody:*:65534:65534::0:0:Unprivileged user:/nonexistent:/usr/sbin/nologin^M
john:<password>:1001:1001::0:0:John Baldwin:/home/john:/bin/tcsh^M
messagebus:*:556:556::0:0:D-BUS Daemon User:/nonexistent:/usr/sbin/nologin^M
polkit:*:562:562::0:0:PolicyKit User:/nonexistent:/usr/sbin/nologin^M
haldaemon:*:560:560::0:0:HAL Daemon User:/nonexistent:/usr/sbin/nologin^M

#^M
wheel:*:0:root,john^M
daemon:*:1:^M
kmem:*:2:^M
sys:*:3:^M
tty:*:4:^M
operator:*:5:root^M
_dhcp:*:65:^M
uucp:*:66:^M
dialer:*:68:^M
network:*:69:^M
audit:*:77:^M
www:*:80:^M
hast:*:845:^M
nogroup:*:65533:^M
nobody:*:65534:^M
john:*:1001:^M
messagebus:*:556:^M
polkit:*:562:^M
haldaemon:*:560:^M

vs

#^M
wheel:*:0:root,john^M
daemon:*:1:^M
kmem:*:2:^M
sys:*:3:^M
tty:*:4:^M
operator:*:5:root^M
_dhcp:*:65:^M
uucp:*:66:^M
dialer:*:68:^M
network:*:69:^M
audit:*:77:www:*:80:^M
hast:*:845:^M
nogroup:*:65533:^M
nobody:*:65534:^M
john:*:1001:^M
messagebus:*:556:^M
polkit:*:562:^M
haldaemon:*:560:^M

A simplified testcase:

$ echo "www:*:80:80:World Wide Web Owner:/nonexistent:/usr/sbin/nologin" | sed -e '/:80:/i\
auditdistd:*:78:77::0:0:Auditdistd unprivileged user:/var/empty:/usr/sbin/nologin'
auditdistd:*:78:77::0:0:Auditdistd unprivileged user:/var/empty:/usr/sbin/nologinwww:*:80:80:World Wide Web Owner:/nonexistent:/usr/sbin/nologin
Comment 6 Mikhail Teterin freebsd_committer 2016-07-27 20:01:53 UTC
(In reply to Ngie Cooper from comment #5)
> A simplified testcase

Ngie, it is a little hard to distinguish Bugzilla's line-wrappings from the intended EOLs... Here is what I just tested:

% cat t-211399
/:80:/i\
auditdistd:*:78:77::0:0:Auditdistd unprivileged user:/var/empty:/usr/sbin/nologin
% echo "www:*:80:80:World Wide Web Owner:/nonexistent:/usr/sbin/nologin" | \
  sed -f t-211399
auditdistd:*:78:77::0:0:Auditdistd unprivileged user:/var/empty:/usr/sbin/nologin
www:*:80:80:World Wide Web Owner:/nonexistent:/usr/sbin/nologin

Using gsed instead of sed produces the exact same output -- which seems correct. Could you, please, refine the example?

That said, I'm testing on 10.x -- where I've developed the patch in Bug #195929. Maybe, the -current got a slightly different change, which would account for the differences...
Comment 7 Pedro F. Giffuni freebsd_committer 2016-07-27 20:23:39 UTC
(In reply to Mikhail Teterin from comment #6)

Just checking ..we don't have FreeBSD-12 reference machines yet :(.
The version from the current tree should build just fine on
FreeBSD 10/11 for testing purposes though.
Comment 8 Pedro F. Giffuni freebsd_committer 2016-07-28 18:26:33 UTC
(In reply to Ngie Cooper from comment #5)
So .. two questions:

Is this a real issue(TM)? It would appear the new files are ase usable as the original ones .. but maybe I am missing something.

What does GNU sed do on this test?
Comment 9 Enji Cooper freebsd_committer 2016-07-29 00:22:55 UTC
The key difference is that gsed and versions of sed prior to r303047 printed out the newline separating the password entry for auditdistd and www. sed after r303047 however omits that newline.

Examples below:

CentOS 6.5:

% echo "www:*:80:80:World Wide Web Owner:/nonexistent:/usr/sbin/nologin" | sed -e '/:80:/i\
auditdistd:*:78:77::0:0:Auditdistd unprivileged user:/var/empty:/usr/sbin/nologin'
auditdistd:*:78:77::0:0:Auditdistd unprivileged user:/var/empty:/usr/sbin/nologin
www:*:80:80:World Wide Web Owner:/nonexistent:/usr/sbin/nologin
% cat /etc/redhat-release
CentOS release 6.5 (Final)

FreeBSD after r303047:

# echo "www:*:80:80:World Wide Web Owner:/nonexistent:/usr/sbin/nologin" | sed -e '/:80:/i\
> auditdistd:*:78:77::0:0:Auditdistd unprivileged user:/var/empty:/usr/sbin/nologin'
auditdistd:*:78:77::0:0:Auditdistd unprivileged user:/var/empty:/usr/sbin/nologinwww:*:80:80:World Wide Web Owner:/nonexistent:/usr/sbin/nologin
# uname -a
FreeBSD fattw-3-2 12.0-CURRENT FreeBSD 12.0-CURRENT #12 r303199+8d3c361(isilon-atf): Fri Jul 22 13:41:49 PDT 2016     ngie@wkstn-fbsd-ngie:/usr/obj/usr/src/sys/GENERIC  amd64

Something seems to have changed on FreeBSD CURRENT -- some older custom versions of FreeBSD I have access to (based on r29xxxxx) don't necessarily exhibit this issue. I need to dig through a bit further to figure out why.
Comment 10 Pedro F. Giffuni freebsd_committer 2016-07-29 02:28:23 UTC
(In reply to Ngie Cooper from comment #9)

There have been many changes in before r303047 (all are in 11-stable) to make our sed is somewhat more similar to gsed without hurting standards. r302973 was not fully tested and may need further reviews but if the tests pass in 11-stable that is not the culprit.

If reverting r303047 (which is only in -current) is the only requirement to keep consistency with gsed and previous sed, let's do it.
Comment 11 Mikhail T. 2016-07-29 03:17:16 UTC
If the problem is a missing \n, I may have an idea. But would still appreciate a concise test case. Can the revert wait for a couple of days? Thanks!
Comment 12 Pedro F. Giffuni freebsd_committer 2016-07-29 03:29:24 UTC
(In reply to Mikhail T. from comment #11)
Yeah, we can wait ... thanks for looking at it.
Comment 13 Mikhail T. 2016-07-30 16:17:21 UTC
Created attachment 173122 [details]
Test-case

Ok, here is the simpler test-case...

gsed and older seds all print out two lines:

abcd
1234

The new sed prints out one line:

abcd1234

Now, this is a bug simply because it is _different_ from the established behavior. But I wonder, if the established behavior is correct -- if the closing quote on sed's command-line is on a new line, the new sed will print the two lines. The old seds do not change their behavior regardless of where the closing quote is. So the new code gives the coder a choice, whereas the old one did not...

Both the old and the new sed will ignore any _additional_ new lines before the closing quote.

I'll try to fix it now.
Comment 14 Pedro F. Giffuni freebsd_committer 2016-07-30 17:19:19 UTC
(In reply to Mikhail T. from comment #13)
Do check the code ngie@ added in phabricator: it's in the URL for this PR.
Comment 15 Mikhail T. 2016-07-30 18:26:19 UTC
Created attachment 173125 [details]
Fix and then some

> Do check the code ngie@ added in phabricator

Ngie's proposal is a work-around, not a fix. He unconditionally adds \n to the argument, duplicating it entirely in the process :)

The full patch, attached, fixes the problem and also makes use of the tlen-field to avoid calling strlen() in process.c. I change the type of tlen from size_t to int, because that's what printf wants to see.

I'd like us to begin using the lengths already set by compile.c so that, some day, the lines may come straight from the mmap-ed script instead of being copies.

The bare minimum required to fix the problem in the PR is just one hunk from the above:

Index: compile.c
===================================================================
--- compile.c   (revision 303554)
+++ compile.c   (working copy)
@@ -753,7 +753,10 @@
                                err(1, "realloc");
                }
        }
-       text[size] = '\0';
+       /* Ensure, there is a new line at the end: */
+       if (text[size - 1] != '\n') {
+               text[size++] = '\n';
+       }
        if ((text = realloc(text, size + 1)) == NULL)
                err(1, "realloc");
        *ptlen = size;

Both ours (multi_test_ and GNU's test-suits pass with either the above minimum hunk or the bigger patch in the attachment.
Comment 16 Mikhail T. 2016-07-30 19:37:52 UTC
Err, sorry, the minimum hunk is not enough by itself -- not without the terminating '\0'. If you wish, I can come up with the smaller patch, but the attachment #173125 [details] is preferable.
Comment 17 Pedro F. Giffuni freebsd_committer 2016-07-30 19:50:29 UTC
(In reply to Mikhail T. from comment #16)

I do prefer Mikhail's approach, ngie can you confirm/commit?
Comment 18 Enji Cooper freebsd_committer 2016-07-31 00:44:10 UTC
(In reply to Pedro F. Giffuni from comment #17)

The attached patch doesn't work. The way I attached to phabricator is the only way I know of (so far) that fixes the behavior to not regress from the pre-r303047 (all I did in the phabricator review was restore the code in a less hacky way by using asprintf instead of malloc+strcpy+strcat).

$ sudo kyua -k /usr/tests/usr.sbin/etcupdate/tests/Kyuafile
always_test:main  ->  passed  [1.805s]
conflicts_test:main  ->  passed  [0.379s]
fbsdid_test:main  ->  passed  [0.424s]
ignore_test:main  ->  passed  [0.153s]
preworld_test:main  ->  failed: Returned non-success exit status 1  [0.147s]
tests_test:main  ->  passed  [1.988s]
tzsetup_test:main  ->  passed  [0.169s]

Results file id is usr_tests_usr.sbin_etcupdate.20160731-004120-932624
Results saved to /root/.kyua/store/results.usr_tests_usr.sbin_etcupdate.20160731-004120-932624.db

6/7 passed (1 failed)
*** Error code 1

Stop.
make: stopped in /usr/src/svn/usr.sbin/etcupdate/tests
$
Comment 19 Mikhail T. 2016-07-31 01:55:18 UTC
(In reply to Ngie Cooper from comment #18)
> The attached patch doesn't work.

Could you post an actual test-case, please? Or is the one I included (attachment #173122 [details]) still failing in your environment? Thanks!
Comment 20 Enji Cooper freebsd_committer 2016-07-31 05:21:42 UTC
(In reply to Mikhail T. from comment #19)

- A repro is provided in comment # 0 using kyua debug.
- The patch you provided is failing the test shown in comment # 0.

This has been broken for 10 days now and has been spamming freebsd-current@ with failure emails. I will commit my change noted in phabricator and we can work out "making it better" later.
Comment 21 commit-hook freebsd_committer 2016-07-31 05:31:28 UTC
A commit references this bug:

Author: ngie
Date: Sun Jul 31 05:31:10 UTC 2016
New revision: 303572
URL: https://svnweb.freebsd.org/changeset/base/303572

Log:
  Fix regression with /i caused by r303047

  '\n' was specifically added to -e arguments prior to r303047. Restore
  historical behavior which in turn fixes usr.sbin/etcupdate/preworld_test:main .

  The fix is being committed to address the issue in the short term and may be
  iterated upon as noted in bug 211399

  Discussed with:		mi, pfg
  Differential Revision:	https://reviews.freebsd.org/D7368
  PR:			195929, 211399 [*]
  MFC after:		18 days
  X-MFC with:		r303047
  Reported by:		Jenkins
  Sponsored by:		EMC / Isilon Storage Division

Changes:
  head/usr.bin/sed/main.c
Comment 22 Mikhail T. 2016-07-31 13:55:12 UTC
(In reply to Ngie Cooper from comment #20)
> - A repro is provided in comment # 0 using kyua debug.

No, that's not a test-case. You offered one in comment #5, which I trimmed further down into attachment #173122 [details].

Unfortunately, you chose to ignore the question I asked about it in comment #19 and took this less than cooperative attitude.

I ask the question again: is the script I posted as attachment #173122 [details] producing joined (incorrect) or separate (correct) lines of output, when given a path to the sed-executable built with patch in attachment #173125 [details] applied (but without your base r303572)?

Thank you.
Comment 23 Enji Cooper freebsd_committer 2017-01-03 06:27:06 UTC
Hasn't failed for some time. Closing.