refactor(cp): replace println! with write_stdout_line for robust error#9901
refactor(cp): replace println! with write_stdout_line for robust error#9901mattsu2020 wants to merge 17 commits intouutils:mainfrom
Conversation
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
|
OK. Even so, the function you added to uucore looks helpful for them. |
src/uucore/src/lib/mods/display.rs
Outdated
| /// This function handles non-UTF-8 environment variable names and values correctly by using | ||
| /// raw bytes on Unix systems. | ||
| pub fn print_all_env_vars<T: fmt::Display>(line_ending: T) -> io::Result<()> { | ||
| crate::check_stdout_write(1)?; |
There was a problem hiding this comment.
please add a comment explaining why this line
it isn't obvious
There was a problem hiding this comment.
the comment doesn't help me to understand why you are adding this here
There was a problem hiding this comment.
At startup
stdout_is_closed() becomes true, so stdout_was_closed is true.
During output
print_all_env_vars repeatedly calls stdout.write_all_os(...).
This uses OsWrite, so it does not go through TrackingWriter.
Without check_stdout_write, stdout_was_written is never set.
During final flush
stdout.flush() may fail with EBADF, etc.
If stdout_was_written == false, it takes the branch that ignores flush errors for closed stdout.
Result
Even though output was attempted and failed,
the error is silently ignored, and it can exit with code 0.
In cases like the following, exit 0 may occur.
( exec 1>&-; env -i FOO=bar ./target/debug/printenv -0 ); echo "exit=$?"
|
GNU testsuite comparison: |
This comment was marked as resolved.
This comment was marked as resolved.
|
GNU testsuite comparison: |
sylvestre
left a comment
sylvestre
left a comment
There was a problem hiding this comment.
sorry but i need more time to think about it
i am not convinced by the changes src/uucore/src/lib/lib.rs
mattsu2020
force-pushed
the
misc/close-stdout.sh
branch
from
aa90754 to
5513284
Compare
Merging this PR will not alter performance
Comparing 
Footnotes
|
|
GNU testsuite comparison: |
…r handling Introduce a new `write_stdout_line` function that locks stdout and returns a Result to handle write failures gracefully. Replace all `println!` calls in verbose output, debug messages, and path printing with this function, propagating errors via `CopyResult<()>` instead of panicking on I/O errors. This improves reliability when stdout is closed or encounters issues during cp operations.
…justing exit code - Made `code` mutable to allow modification on flush errors. - Ignore BrokenPipe errors during stdout flush to avoid breaking utilities that silence it (e.g., seq). - Set exit code to 1 if a flush error occurs and the original code was 0, treating write errors as failures.
- Added check to capture if stdout was closed before utility execution using fcntl on Unix. - Modified flush error handling to ignore EBADF errors when stdout was pre-closed, avoiding false failures for silent commands.
Reorder imports for consistency and refactor the ignore_closed_stdout condition to a more concise form, improving code readability without altering functionality.
Add `stdout_is_closed()` and `is_closed_stdout_error()` functions to check if stdout is closed and detect write errors due to closed stdout. Refactor the `bin!` macro to use these functions, reducing code duplication and improving reusability across utilities.
Implement mechanism to detect if stdout was closed at process start and track write attempts. Add `check_stdout_write()` to error on writes when stdout is closed, preventing issues in utilities like cp and printf. Use `TrackingWriter` in printf for write monitoring. This improves robustness in scenarios like pipeline redirections or closed stdout.
…lity - Condensed multi-line cfg_attr into a single line in early_stdout_state mod - Split long variable assignment across lines in bin macro for better readability This improves code consistency and readability without changing functionality.
Add a check to ensure stdout is writable before printing environment variables, preventing errors when stdout is closed. This uses a non-zero sentinel to trigger the check and mark write attempt.
- Updated the comment to better explain that the path writes via OsWrite, bypassing TrackingWriter, thus requiring an early check to mark stdout as "written" and fail if fd 1 is already closed. This improves code readability and understanding of the stdout-closed handling logic.
Implement atomic-based stdout state management in cp and printf utilities to detect if stdout was closed before writing, avoiding errors in broken pipe scenarios (e.g., when piped output is terminated early). Added libc dependency for Unix-specific fcntl checks, ensuring proper error handling on platforms where stdout closure can occur.
Add 'devnull' to the spell-checker ignore comments in src/uu/cp/src/cp.rs and src/uu/printf/src/printf.rs to prevent false positives, as 'devnull' refers to the /dev/null device and is a valid technical term in the codebase.
Update various crates in fuzz/Cargo.lock to their latest versions, including blake2b_simd, blake3, cc, clap_lex, constant_time_eq, crc-fast, data-encoding, and others for improved performance, security fixes, and compatibility.
… readability - Split arguments onto multiple lines in both cp.rs and printf.rs to improve code formatting and consistency. No functional changes.
mattsu2020
force-pushed
the
misc/close-stdout.sh
branch
from
6c6acf3 to
bf227bd
Compare
|
GNU testsuite comparison: |
Add tests to verify that `cp` and `printf` utilities handle stdout failures correctly. Specifically, test cases are added to check behavior when stdout is closed or redirected to `/dev/full`. These tests ensure that the utilities fail gracefully and provide appropriate error messages when stdout operations cannot be completed.
|
GNU testsuite comparison: |
|
We now already have the ability to read the state of the stdin stdout and stderr and its called at the beginning of all of the utilities, we do not have to create that behavior again in a new library. I think the majority of the handling of these gnu test cases can be handled in the error handling library with the already existing. This would require us to swap println with writeln and add the error handler ? but it wouldn't require adding a whole new library for writing output. Heres an example branch that shows that it can pass the gnu tests with just the change from println to writeln: https://github.com/ChrisDryden/coreutils/tree/fix-cp-stdout-write-errors |
4 participants
Introduce a new
write_stdout_linefunction that locks stdout and returns a Result to handle write failures gracefully. Replace allprintln!calls in verbose output, debug messages, and path printing with this function, propagating errors viaCopyResult<()>instead of panicking on I/O errors. This improves reliability when stdout is closed or encounters issues during cp operations.related
misc/close-stdout.sh