Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

shuf: Fix off-by-one errors in range handling #6014

Merged
merged 2 commits into from
Mar 10, 2024

Conversation

BenWiederhake
Copy link
Collaborator

It seems that I accidentally introduced an off-by-one error in #5980. My bad!

This PR:

  • Fixes the off-by-one error by using the standard type RangeInclusive<usize>, instead of passing (usize, usize) around and hoping that we don't forget whether the range is inclusive or exclusive. (That was the bug.)
  • Adds a bunch of tests to make sure this cannot regress. (This is very relevant: shuf is somewhat inefficient, especially when permuting a reasonably-large number range.)
  • Requires shuf: Refuse repeating zero lines #6011, hence this PR is a draft until shuf: Refuse repeating zero lines #6011 lands
  • Adds an intentional GNU behavior bug, see below.
  • Does not touch the existing implicit bug around -n: fn parse_head_count defaults to std::usize::MAX, which is technically a bug, but has no practical impact.

Sadly, it seems that GNU shuf is buggy:

$ shuf -r -n1 -i 0-18446744073709551614
13338223435012857584
$ shuf -r -n1 -i 1-18446744073709551615
1293781479722686075
$ shuf -r -n1 -i 0-18446744073709551615 # BUG!
shuf: invalid input range: '0-18446744073709551615'
[$? = 1]
$ shuf -n1 -i 0-18446744073709551614
5447107942308837389
$ shuf -n1 -i 1-18446744073709551615
2689384841796966305
$ shuf -n1 -i 0-18446744073709551615 # BUG!
shuf: invalid input range: '0-18446744073709551615'
[$? = 1]

I really hope we do not try to be bug-compatible. This PR results in:

$ cargo run shuf -n1 -i 0-18446744073709551615
1580802165403567859
$ cargo run shuf -r -n1 -i 0-18446744073709551615
7712157147661886880

Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/shuf/shuf is no longer failing!
Skipping an intermittent issue tests/tail/inotify-dir-recreate (passes in this run but fails in the 'main' branch)

@BenWiederhake BenWiederhake marked this pull request as ready for review February 27, 2024 22:54
@BenWiederhake BenWiederhake force-pushed the dev-shuf-range-off-by-one branch from 9549bae to 5fe4ed1 Compare February 27, 2024 22:54
@BenWiederhake
Copy link
Collaborator Author

Changes since last push:

@BenWiederhake
Copy link
Collaborator Author

Looks like we accidentally found a bug in Windows BFD, because it says BFD (GNU Binutils) 2.39 assertion fail:

2024-02-27T23:00:03.8935508Z    Compiling hex-literal v0.4.1
2024-02-27T23:00:03.9829797Z    Compiling unindent v0.2.1
2024-02-27T23:00:20.1646720Z error: linking with `x86_64-w64-mingw32-gcc` failed: exit code: 1
2024-02-27T23:00:20.1648606Z   |
2024-02-27T23:00:20.1921863Z   = note: "x86_64-w64-mingw32-gcc" "-fno-use-linker-plugin" "-Wl,--dynamicbase" "-Wl,--disable-auto-image-base" "-m64" …INCREDIBLY MANY OPTIONS… "-o" "D:\\a\\coreutils\\coreutils\\target\\debug\\deps\\coreutils-49fd98cd30ebb250.exe" "-no-pie" "-nodefaultlibs" "C:\\Users\\runneradmin\\.rustup\\toolchains\\nightly-x86_64-pc-windows-gnu\\lib\\rustlib\\x86_64-pc-windows-gnu\\lib\\rsend.o"
2024-02-27T23:00:20.2725618Z   = note: Warning: .drectve `-exclude-symbols:"_ZN100_$LT$alloc..string..String$u20$as$u20$core..ops..index..Index$LT$core..ops..range..RangeFull$GT$$GT$5index17h4cf8051789717498E" ' unrecognized
2024-02-27T23:00:20.2730336Z           Warning: .drectve `-exclude-symbols:"_ZN100_$LT$clap_builder..builder..value_parser..EnumValueParser$LT$E$GT$$u20$as$u20$core..clone..Clone$GT$5clone17h46c7d57fd48394ffE" ' unrecognized
2024-02-27T23:00:20.2733227Z           Warning: .drectve `-exclude-symbols:"_ZN100_$LT$core..iter..adapters..fuse..Fuse$LT$I$GT$$u20$as$u20$core..iter..traits..iterator..Iterator$GT$4next17h4a14f274301286fbE" ' unrecognized

… TWO HUNDRED THOUSAND LINES OF "WARNING CORRPT DRECTVE" …

2024-02-27T23:00:38.8349081Z           C:/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: BFD (GNU Binutils) 2.39 assertion fail ../../../src/binutils-2.39/bfd/coffgen.c:460
2024-02-27T23:00:38.8350405Z           C:/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: warning: C:\Users\runneradmin\.cargo\registry\src\index.
crates.io-6f17d22bba15001f\windows_x86_64_gnu-0.42.2\lib/libwindows.a(ntdll_dll_s00082.o): local symbol `' has no section
2024-02-27T23:00:38.8351203Z           C:/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: BFD (GNU Binutils) 2.39 assertion fail ../../../src/binutils-2.39/bfd/coffgen.c:460
2024-02-27T23:00:38.8352046Z           C:/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: BFD (GNU Binutils) 2.39 assertion fail ../../../src/binutils-2.39/bfd/cofflink.c:2279
2024-02-27T23:00:38.8353343Z           C:/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: C:\Users\runneradmin\.cargo\registry\src\index.crates.io
-6f17d22bba15001f\windows_x86_64_gnu-0.42.2\lib/libwindows.a(ntdll_dll_s00082.o): illegal symbol index -1869611008 in relocs
2024-02-27T23:00:38.8353651Z           collect2.exe: error: ld returned 1 exit status
2024-02-27T23:00:38.8354142Z error: could not compile `coreutils` (test "tests") due to 1 previous error

Yet another failed CI that has to be ignored.

@sylvestre sylvestre force-pushed the dev-shuf-range-off-by-one branch from 5fe4ed1 to b233569 Compare March 9, 2024 21:51
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/chown/preserve-root is no longer failing!

@sylvestre sylvestre merged commit 80702d5 into uutils:main Mar 10, 2024
62 checks passed
@BenWiederhake BenWiederhake deleted the dev-shuf-range-off-by-one branch March 11, 2024 01:52
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants