Bug 270867 - xargs -E is not working properly
Summary: xargs -E is not working properly
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Many People
Assignee: Yuri Pankov
URL: https://reviews.freebsd.org/D39583
Keywords:
Depends on:
Blocks:
 
Reported: 2023-04-15 15:49 UTC by Koichi Nakashima
Modified: 2024-01-10 04:43 UTC (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Koichi Nakashima 2023-04-15 15:49:50 UTC
With the -E option, an empty line or a line beginning with a space terminates the readout there.

Expected 1:  
$ printf '%s\n' a b '' d e | xargs -Ex
a b d e 

Actual 1:
$ printf '%s\n' a b '' d e | xargs -Ex
a b

Expected 2:  
$ printf '%s\n' a b ' c' d e | xargs -Ex
a b c d e 

Actual 2:
$ printf '%s\n' a b ' c' d e | xargs -Ex
a b

Perhaps a related issue, when two or more characters are specified in the -E option, the reading is terminated with a string different from the specified string.

Expected 3:  
$ printf '%s\n' a b c d e | xargs -Ecc
a b c d e

Actual 2:
$ printf '%s\n' a b c d e | xargs -Ecc
a b
Comment 1 Yuri Pankov freebsd_committer freebsd_triage 2023-04-15 16:17:13 UTC
Apparently foundeof check is passing the wrong length to strncmp(), we should be using the eofstr's one, not the current argument's:

diff --git a/usr.bin/xargs/xargs.c b/usr.bin/xargs/xargs.c
index 21455e7be26..89f75a4c386 100644
--- a/usr.bin/xargs/xargs.c
+++ b/usr.bin/xargs/xargs.c
@@ -349,7 +349,7 @@ arg1:               if (insingle || indouble) {
                }
 arg2:
                foundeof = *eofstr != '\0' &&
-                   strncmp(argp, eofstr, p - argp) == 0;
+                   strncmp(argp, eofstr, strlen(eofstr)) == 0;

                /* Do not make empty args unless they are quoted */
                if ((argp != p || wasquoted) && !foundeof) {
Comment 2 Yuri Pankov freebsd_committer freebsd_triage 2023-04-15 16:21:11 UTC
(In reply to Yuri Pankov from comment #1)
Or even just strcmp() so that 'ccc' is not acknowledged as eof with -Ecc?
Comment 3 Yuri Pankov freebsd_committer freebsd_triage 2023-04-15 16:33:02 UTC
(In reply to Yuri Pankov from comment #2)
And there's a commit that changed strcmp() into strncmp():

commit 4aeb63826e0f9ce817b16abdb1501490307d857c
Author: Juli Mallett <jmallett@FreeBSD.org>
Date:   Sat Dec 31 09:06:45 2005 +0000

    Check the entire length of the current argument for the EOF string with -E,
    but don't expect a proper ASCII string to exist right here right now, don't
    use strcmp(3) which checks for a NUL.  As we're still building the argument
    up, the next character might be garbage.  It would probably be just as safe to
    temporarily write a NUL there, but if we've reached the end of argument memory
    that might not be the best idea, I think.  It's unclear.

    Doing it this way seems to meet the most with the original intent.

    PR:             85696
    Prodded by:     stefanf

Checking if what's described in bug 85696 is still a problem with strcmp()...
Comment 4 Yuri Pankov freebsd_committer freebsd_triage 2023-04-15 17:32:50 UTC
Sorry for being verbose in here, the proposed fix is at: https://reviews.freebsd.org/D39583.
Comment 5 commit-hook freebsd_committer freebsd_triage 2023-04-17 20:22:18 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=21ef48af5c0f4ed85a5f42855b5a8d58b5431103

commit 21ef48af5c0f4ed85a5f42855b5a8d58b5431103
Author:     Yuri Pankov <yuripv@FreeBSD.org>
AuthorDate: 2023-04-17 19:14:33 +0000
Commit:     Yuri Pankov <yuripv@FreeBSD.org>
CommitDate: 2023-04-17 20:20:03 +0000

    xargs: improve foundeof check for -E

    4aeb63826e0f got it almost correct (we can't use strcmp() here as
    current argument isn't guaranteed to be NUL-terminated), but we also
    need to check that current argument length is equal to that of eofstr.

    PR:             270867
    Reviewed by:    bapt
    Differential Revision:  https://reviews.freebsd.org/D39583

 usr.bin/xargs/xargs.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
Comment 6 commit-hook freebsd_committer freebsd_triage 2023-06-14 13:27:30 UTC
A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=bc47005a648b1b326cfd852c039a9b01854c5796

commit bc47005a648b1b326cfd852c039a9b01854c5796
Author:     Tom Jones <thj@FreeBSD.org>
AuthorDate: 2022-07-05 15:03:51 +0000
Commit:     Dag-Erling Smørgrav <des@FreeBSD.org>
CommitDate: 2023-06-14 12:48:55 +0000

    xargs: terminate if line replacement cannot be constructed

    If the line with replacement cannot be constructed xargs should
    terminate as documented in the man page

    We encounter this error, but gnu/xargs doesn't because they have a much
    larger limit for created outputs (~10000 lines).

    Reviewed by:    oshogbo
    Sponsored by:   Klara, Inc.
    Differential Revision:  https://reviews.freebsd.org/D35574

    (cherry picked from commit f058359ba5e08c555d7e6f192217f890b83cd46c)

    xargs: fix description of strnsubst return value

    Reported by:    oshogbo
    Sponsored by:   Klara, Inc.
    Differential Revision:  https://reviews.freebsd.org/D35574

    (cherry picked from commit 1e692b938e37a9b43a43ace2739eb6b97379cac0)

    xargs: improve foundeof check for -E

    4aeb63826e0f got it almost correct (we can't use strcmp() here as
    current argument isn't guaranteed to be NUL-terminated), but we also
    need to check that current argument length is equal to that of eofstr.

    PR:             270867
    Reviewed by:    bapt
    Differential Revision:  https://reviews.freebsd.org/D39583

    (cherry picked from commit 21ef48af5c0f4ed85a5f42855b5a8d58b5431103)

    xargs: Fix typo in error message.

    MFC after:      1 week
    Sponsored by:   Klara, Inc.

    (cherry picked from commit 6d777389e19d3bebde515e88e8405de45d8af7bd)

    xargs: Consistently use strtonum() to parse arguments.

    MFC after:      1 week
    Sponsored by:   Klara, Inc.
    Reviewed by:    kevans
    Differential Revision:  https://reviews.freebsd.org/D40425

    (cherry picked from commit fbc445addf9183d180bb8b488281617bb19d9242)

 usr.bin/xargs/strnsubst.c | 15 ++++++++++-----
 usr.bin/xargs/xargs.c     | 44 +++++++++++++++++++++++++-------------------
 2 files changed, 35 insertions(+), 24 deletions(-)
Comment 7 Mark Linimon freebsd_committer freebsd_triage 2024-01-10 04:43:36 UTC
^Triage: assign to committer that resolved.

des@ has MFCed to 13.