coreutils icon indicating copy to clipboard operation
coreutils copied to clipboard

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

Open BenWiederhake opened this issue 1 year ago • 1 comments

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 #6011, hence this PR is a draft until #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

BenWiederhake avatar Feb 25 '24 13:02 BenWiederhake

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)

github-actions[bot] avatar Feb 25 '24 14:02 github-actions[bot]

Changes since last push:

  • Rebased, now that #6011 has landed
  • Fixed and added a test for the new issue found by @cakebaker – Thanks!

BenWiederhake avatar Feb 27 '24 22:02 BenWiederhake

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.

BenWiederhake avatar Feb 27 '24 23:02 BenWiederhake

GNU testsuite comparison:

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

github-actions[bot] avatar Mar 10 '24 01:03 github-actions[bot]