Bug 252542 - cmp -s + regular files + skipping is broken
Summary: cmp -s + regular files + skipping is broken
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: Unspecified
Hardware: Any Any
: --- Affects Some People
Assignee: Ed Maste
URL: https://reviews.freebsd.org/D28071
Keywords:
Depends on:
Blocks:
 
Reported: 2021-01-09 10:35 UTC by William Ahern
Modified: 2021-01-15 15:10 UTC (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description William Ahern 2021-01-09 10:35:15 UTC
The file length comparison short-circuit in cmp.c for when the -s flag, regular files, and SKIP1 are specified is broken. c_regular in regular.c had a similar buggy short-circuit that was fixed in June 2000. But in July 2000 the -z flag feature was added which accidentally reintroduced the same bug. The bug is comparing the file sizes without accounting for skip1 and skip2. See https://svnweb.freebsd.org/base/head/usr.bin/cmp/cmp.c?revision=344551&view=markup#l193 Note that both sflag and zflag are set when -s is specified. 

Discovered when investigating a bug report on the OpenBSD mailing-list. More detailed dissection at https://marc.info/?l=openbsd-misc&m=161017967207304&w=2
Comment 1 Ed Maste freebsd_committer freebsd_triage 2021-01-09 21:48:34 UTC
In the man page,
     -z      For regular files compare file sizes first, and fail the
             comparison if they are not equal.
so I think this is expected behaviour with -z; do you agree?

For -s this is certainly a bug; I'm considering this as a fix:


diff --git a/usr.bin/cmp/cmp.c b/usr.bin/cmp/cmp.c
index c762f1346abf..47f9b671985c 100644
--- a/usr.bin/cmp/cmp.c
+++ b/usr.bin/cmp/cmp.c
@@ -92,7 +92,6 @@ main(int argc, char *argv[])
                        break;
                case 's':               /* silent run */
                        sflag = true;
-                       zflag = true;
                        break;
                case 'x':               /* hex output */
                        lflag = true;
@@ -149,6 +148,9 @@ main(int argc, char *argv[])
        skip1 = argc > 2 ? strtol(argv[2], NULL, 0) : 0;
        skip2 = argc == 4 ? strtol(argv[3], NULL, 0) : 0;
 
+       if (sflag && skip1 == 0 && skip2 == 0)
+               zflag = true;
+
        if (fd1 == -1) {
                if (fd2 == -1) {
                        c_link(file1, skip1, file2, skip2);
Comment 2 Ed Maste freebsd_committer freebsd_triage 2021-01-09 22:09:09 UTC
Please see the review in https://reviews.freebsd.org/D28071
Comment 3 commit-hook freebsd_committer freebsd_triage 2021-01-11 00:05:16 UTC
A commit in branch main references this bug:

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

commit 80445b7a3f738e0b0a33ee7a11905a275346a6de
Author:     Ed Maste <emaste@FreeBSD.org>
AuthorDate: 2021-01-11 00:02:56 +0000
Commit:     Ed Maste <emaste@FreeBSD.org>
CommitDate: 2021-01-11 00:02:56 +0000

    cmp: fix -s (silent) when used with skip offsets

    -s causes cmp to print nothing for differing files, for use when only
    the exit status is of interest.

    -z compares the file size first, for regular files, and fails the
    comparison early if they do not match.

    Prior to this change -s implied -z as an optimization, but this is not
    valid when file offsets are specified.  Now, enable the -z optimization
    for -s only if both skip arguments are not provided / 0.

    Note that using -z with differing skip values will currently always
    fail.  We may want to compare size1 - skip1 with size2 - skip2 instaead,
    and in any case the man page should be clarified.

    PR:             252542
    Fixes:          3e6902efc802ab57fc4e9bf798f2d271b152e7f9
    Reported by:    William Ahern
    Reviewed by:    markj
    MFC after:      1 week
    Sponsored by:   The FreeBSD Foundation
    Differential Revision:  https://reviews.freebsd.org/D28071

 usr.bin/cmp/cmp.c              |  4 +++-
 usr.bin/cmp/tests/cmp_test2.sh | 14 ++++++++++++++
 2 files changed, 17 insertions(+), 1 deletion(-)
Comment 4 commit-hook freebsd_committer freebsd_triage 2021-01-15 14:27:23 UTC
A commit in branch stable/12 references this bug:

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

commit 23b5fa56a182574fef416caeb55e6c735087e411
Author:     Ed Maste <emaste@FreeBSD.org>
AuthorDate: 2021-01-11 00:02:56 +0000
Commit:     Ed Maste <emaste@FreeBSD.org>
CommitDate: 2021-01-15 14:25:36 +0000

    cmp: fix -s (silent) when used with skip offsets

    -s causes cmp to print nothing for differing files, for use when only
    the exit status is of interest.

    -z compares the file size first, for regular files, and fails the
    comparison early if they do not match.

    Prior to this change -s implied -z as an optimization, but this is not
    valid when file offsets are specified.  Now, enable the -z optimization
    for -s only if both skip arguments are not provided / 0.

    Note that using -z with differing skip values will currently always
    fail.  We may want to compare size1 - skip1 with size2 - skip2 instaead,
    and in any case the man page should be clarified.

    PR:             252542
    Fixes:          3e6902efc802ab57fc4e9bf798f2d271b152e7f9
    Reported by:    William Ahern
    Reviewed by:    markj
    Sponsored by:   The FreeBSD Foundation
    Differential Revision:  https://reviews.freebsd.org/D28071

    (cherry picked from commit 80445b7a3f738e0b0a33ee7a11905a275346a6de)

 usr.bin/cmp/cmp.c              |  4 +++-
 usr.bin/cmp/tests/cmp_test2.sh | 14 ++++++++++++++
 2 files changed, 17 insertions(+), 1 deletion(-)