Bug 271964 - seq(1) printing one line more than it should for many inputs since at least early 2018
Summary: seq(1) printing one line more than it should for many inputs since at least e...
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: Unspecified
Hardware: Any Any
: --- Affects Many People
Assignee: Ed Maste
URL: https://reviews.freebsd.org/D40601
Keywords:
Depends on:
Blocks:
 
Reported: 2023-06-12 11:11 UTC by Daniel Kolesa
Modified: 2024-08-30 21:29 UTC (History)
4 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Kolesa 2023-06-12 11:11:47 UTC
Currently running the following will get you incorrect results:

$ seq 1 1050000|wc -l
 1050001

It comes down to this:

$ seq 1050000 1050000
1.05e+06
1.05e+06

I've been able to track down the offending commit, which is https://github.com/freebsd/freebsd-src/commit/3049d4ccc031e5d4d0ff6b9a2445131f507778f2

As I see it, the logic of that commit is wrong and will result in false positives more often than not in practice. The %g format string which is used by default will truncate any value over 999999. The decimal precision of a double type is much larger than that, as double has 53-bit mantissa. While at it, the condition that 'cur != last_shown_value' will *always* be true, as the 'cur' will *always* need to increment past the end in order to break the loop at all.

I don't see any reliable way to check this, actually. I don't think it is actually possible to check if a rounding error happened or not. Therefore, I think the above change should be reverted, unless somebody has a better idea.

This likely affects other BSDs as well, as it was found thanks to https://floss.social/@Gottox@chaos.social/110527807611326720 - but I'm reporting it here as I work primarily with FreeBSD sources and reverted it in my project (https://github.com/chimera-linux/chimerautils/commit/1ecc1e99d4a309631e846a868b5a422f996704ac).
Comment 1 Daniel Tameling 2023-06-13 11:02:59 UTC
OpenBSD has committed a fix:
https://github.com/openbsd/src/commit/30f0fd29ba6c5cb385908d42cdff89a10af6dfde

The code is very similar and it might be possible to use the same fix.
Comment 2 commit-hook freebsd_committer freebsd_triage 2023-06-19 15:29:12 UTC
A commit in branch main references this bug:

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

commit e54db9a9ccd588f650a2e57bf9d1cbbafc0e12eb
Author:     Ed Maste <emaste@FreeBSD.org>
AuthorDate: 2023-06-19 01:37:06 +0000
Commit:     Ed Maste <emaste@FreeBSD.org>
CommitDate: 2023-06-19 15:02:38 +0000

    seq: fix check for rounding error/truncation

    Based on OpenBSD 30f0fd29ba6c:
    > We need to compare the printable version of the last value displayed,
    > not the floating point representation.  Otherwise, we may print the
    > last value twice.

    PR:             271964
    Reported by:    Daniel Kolesa
    Reviewed by:    yuripv
    Obtained from:  OpenBSD
    Sponsored by:   The FreeBSD Foundation
    Differential Revision:  https://reviews.freebsd.org/D40601

 usr.bin/seq/seq.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)
Comment 3 Daniel Kolesa 2023-06-19 21:01:21 UTC
by the way, relatedly, how about changing the default format string from %g to %.0f in the cases where 1) increment is an integer 2) minimum and maximum values are integers and can be stored in a double exactly (i.e. up to 2^53)?
Comment 4 commit-hook freebsd_committer freebsd_triage 2023-06-26 16:31:38 UTC
A commit in branch stable/13 references this bug:

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

commit 70af4d57c18fbcb46e3d7d7501cad756f812bed3
Author:     Ed Maste <emaste@FreeBSD.org>
AuthorDate: 2023-06-19 01:37:06 +0000
Commit:     Ed Maste <emaste@FreeBSD.org>
CommitDate: 2023-06-26 16:30:31 +0000

    seq: fix check for rounding error/truncation

    Based on OpenBSD 30f0fd29ba6c:
    > We need to compare the printable version of the last value displayed,
    > not the floating point representation.  Otherwise, we may print the
    > last value twice.

    PR:             271964
    Reported by:    Daniel Kolesa
    Reviewed by:    yuripv
    Obtained from:  OpenBSD
    Sponsored by:   The FreeBSD Foundation
    Differential Revision:  https://reviews.freebsd.org/D40601

    (cherry picked from commit e54db9a9ccd588f650a2e57bf9d1cbbafc0e12eb)

 usr.bin/seq/seq.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)
Comment 5 Ed Maste freebsd_committer freebsd_triage 2023-06-28 17:33:01 UTC
(In reply to Daniel Kolesa from comment #3)
> by the way, relatedly, how about changing the default format string from %g to
> %.0f in the cases where 1) increment is an integer 2) minimum and maximum
> values are integers and can be stored in a double exactly (i.e. up to 2^53)?

This does seem reasonable to me, in particular I think the least surprising output for `seq 999999 1 1000001` would be

999999
1000000
1000001

and not

999999
1e+06
1e+06

That said it's best to have this PR refer to the doubled line issue, and thus I'd ask you to submit a patch (if you're able) or open a separate PR for the default format.
Comment 6 Daniel Kolesa 2023-06-28 18:06:06 UTC
sure, i can make and submit a patch
Comment 7 Mark Johnston freebsd_committer freebsd_triage 2023-10-02 16:23:22 UTC
Is there anything left to do to address the original bug?
Comment 8 Ed Maste freebsd_committer freebsd_triage 2024-08-30 21:29:22 UTC
(In reply to Mark Johnston from comment #7)
> Is there anything left to do to address the original bug?

No, I believe everything is done for the original bug, so closing.