df: there should be no trailing spaces in the rightmost column

[cakebaker]

This is a reminder so that the issue doesn't get forgotten after its implementation got reverted in #3442.

$ df | tr " " "_"
Filesystem_____1K-blocks______Used_Available_Use%_Mounted_on
dev______________4006844_________0___4006844___0%_/dev
run______________4014808______1340___4013468___1%_/run
/dev/sda8______458091736_131027036_303771928__31%_/
tmpfs____________4014808_____65704___3949104___2%_/dev/shm
tmpfs____________4014812______8044___4006768___1%_/tmp
/dev/sda2_________262144_____84060____178084__33%_/boot
tmpfs_____________802960________76____802884___1%_/run/user/1000

$ ./target/debug/coreutils df | tr " " "_"
Filesystem_____1K-blocks______Used_Available_Use%_Mounted_on____
dev______________4006844_________0___4006844___0%_/dev__________
run______________4014808______1340___4013468___1%_/run__________
/dev/sda8______458091736_131027036_303771928__31%_/_____________
tmpfs____________4014808_____67880___3946928___2%_/dev/shm______
tmpfs____________4014812______8044___4006768___1%_/tmp__________
/dev/sda2_________262144_____84060____178084__33%_/boot_________
tmpfs_____________802960________76____802884___1%_/run/user/1000

[jfinkels]

To summarize the information in other issues and pull requests:

• Pull request df: remove trailing spaces in rightmost column #3423 introduced code to trim the trailing whitespace in each row of the table produced by df.
• Issue "Run GNU tests" on main fails to complete for the last few days  #3439 reported that the GNU test suite in the CI environment was not terminating.
• Pull request Revert "df: remove trailing spaces in rightmost column" #3442 reverted the fix and the GNU test suite seemed to terminate again as desired.

[sylvestre]

exactly, thanks :)

[jfinkels]

I checked out the branch from #3423 to diagnose why the GNU tests seemed to be running forever. I ran the test case tests/dd/skip-seek-past-dev.sh and it trashed my system, so don't try this at home! That test case starts with

# Get path to device the current dir is on.                                     
# Note df can only get fs size, not device size.                                
device=$(df --output=source . | tail -n1) || framework_failure_

dev_size=$(get_device_size "$device") ||
  skip_ "failed to determine size of $device"

On the main branch, we can see the device names has trailing spaces, which causes this test to be skipped. (The device's name is "/dev/root", not "/dev/root ".) Here are the logs:

2022-04-28T07:17:18.8534023Z SKIP: tests/dd/skip-seek-past-dev
2022-04-28T07:17:18.8534274Z =================================
2022-04-28T07:17:18.8534417Z 
2022-04-28T07:17:18.8534582Z blockdev: cannot open /dev/root     : No such file or directory
2022-04-28T07:17:18.8535093Z skip-seek-past-dev.sh: skipped test: failed to determine size of /dev/root     
2022-04-28T07:17:18.8535570Z SKIP tests/dd/skip-seek-past-dev.sh (exit status: 77)

So I'm thinking that by fixing the bug in df, we caused the tests/dd/skip-seek-past-dev.sh test script to no longer be skipped, and thus we uncovered a (serious) bug in dd.

[sylvestre]

Well spotted, bravo!
sorry for your system btw :(

[jhscheer]

@jfinkels As I am/was suffering from this system crash as well, I'm curious how you pinpointed the issue to gnu/tests/dd/skip-seek-past-dev.sh ?

[jfinkels]

To be honest I'm not absolutely certain that's the problem, but here was my reasoning. I think I searched in gnu/tests/ for all usages of df:

../gnu/tests/ls/readdir-mountpoint-inode.sh
24:df --local --out=target | sed -n '/^\/./p' > mount_points

../gnu/tests/tail-2/end-of-device.sh
34:# Note df can only get fs size, not device size.
35:device=$(df --output=source . | tail -n1) || framework_failure_

../gnu/tests/dd/skip-seek-past-dev.sh
35:# Note df can only get fs size, not device size.
36:device=$(df --output=source . | tail -n1) || framework_failure_

../gnu/tests/cp/sparse-extents-2.sh
26:if seek_data_capable_ sparse_chk && ! df -t ext3 . >/dev/null; then

../gnu/tests/du/2g.sh
29:free_kb=$(df -k --output=avail . | tail -n1)
32:  *) skip_ "invalid size from df: $free_kb";;

../gnu/tests/misc/help-version-getopt.sh
46:    # E.g., "nohup df --help" should run "df --help", not "df --help".
49:      df $opt > exp || framework_failure_
50:      BEFORE=df

../gnu/tests/rm/ext3-perf.sh
43:df -T -t ext3 -t ext4dev -t ext4 . \

and then looked for ones that seemed suspicious. For example, I made an educated guess that the usage in tests/rm/ext3-perf.sh should be no problem, since it is just checking that there is at least one ext3 or ext4 filesystem. The usage in tests/dd/skip-seek-past-dev.sh seemed the most likely suspect because it includes these lines:

timeout 10 dd bs=1 skip=$DEV_OFLOW count=0 status=noxfer < "$device" 2> err
# ...
timeout 10 dd bs=1 seek=$DEV_OFLOW count=0 status=noxfer > "$device" 2> err

I thought this was suspicious because it involves seeking past the entire contents of the filesystem, and stdin/stdout redirection. Something similar is happening in tests/tail-2/end-of-device.sh, so there could be an issue there as well.

I didn't pursue this any further after it crashed my system 😨, so I may be wrong!

[jfinkels]

Here's a slightly deeper hypothesis.

The command

timeout 10 dd bs=1 seek=$DEV_OFLOW count=0 status=noxfer > "$device" 2> err

causes this line of code to get executed:

coreutils/src/uu/dd/src/dd.rs

Line 307 in 8891571

io::copy(&mut io::repeat(0u8).take(amt as u64), &mut dst)

which writes $DEV_OFLOW zeros to the output $device. That's bad if $device is my filesystem 😂

The GNU test case expects an error message dd: 'standard output': cannot seek: Invalid argument so I'm guessing there is some kind of guard against this in the GNU code that we are missing in our code.

[jfinkels]

The line of code in dd.rs that I linked above is in the implementation impl OutputTrait for Output<io::Stdout>. The code comment there is informative:

        // stdout is not seekable, so we just write null bytes.
        if amt > 0 {
            io::copy(&mut io::repeat(0u8).take(amt as u64), &mut dst)
                .map_err_context(|| String::from("write error"))?;
        }

So dd is behaving as though the output file were stdout, even though in the test case we are interested in, stdout is being redirected to /dev/sda1 (or similar).

The Output object is instantiated here:

    match (
        matches.is_present(options::INFILE),
        matches.is_present(options::OUTFILE),
    ) {
        // <other arms elided here>
        (false, false) => {
            let i = Input::<io::Stdin>::new(&matches)?;
            let o = Output::<io::Stdout>::new(&matches)?;
            o.dd_out(i).map_err_context(|| "IO error".to_string())
        }

Perhaps we need some additional code to check whether stdout is being redirected to a block device, and if so, treat it as a seekable (special) file?