Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Writing a sufficiently large string to a git bash pipe on windows regressed in 1.62 #98947

Closed
smmalis37 opened this issue Jul 5, 2022 · 4 comments · Fixed by #98950
Closed
Labels
C-bug Category: This is a bug. O-windows Operating system: Windows P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@smmalis37
Copy link
Contributor

Reproduction steps:

  1. Be on windows.
  2. cargo new --bin something
  3. replace main.rs with main.txt and rename it back to main.rs.
  4. Open git bash
  5. Run cargo run | cat

I expected to see this happen: my really long string gets printed twice, followed by a clean exit

Instead, this happened: some amount of my really long string gets printed, followed by:
fatal runtime error: I/O error: operation failed to complete synchronously

Additional Details

  • The test case, while simple, does appear to have a minimum size requirement to trigger this crash. Shrinking the string will cause it to stop crashing.
  • I'm using cat in the steps for simplicity, but we've observed this happening with other programs at the end of the pipe too.
  • Redirecting to a file instead of a pipe seems to not trigger this crash.
  • Using cargo run is not necessary to trigger the crash, running the built executable directly also crashes.
  • This PR added this new error condition: Fix unsound File methods #95469
  • I am not the only one seeing this new error message: rustc -C help panics (ICEs) when piped to a closed stdout #98700 (comment)

Version it worked on

rustc 1.61.0 (fe5b13d68 2022-05-18)
binary: rustc
commit-hash: fe5b13d681f25ee6474be29d748c65adcd91f69e
commit-date: 2022-05-18
host: x86_64-pc-windows-msvc
release: 1.61.0
LLVM version: 14.0.0

Version with regression

rustc 1.62.0 (a8314ef7d 2022-06-27)
binary: rustc
commit-hash: a8314ef7d0ec7b75c336af2c9857bfaf43002bfc
commit-date: 2022-06-27
host: x86_64-pc-windows-msvc
release: 1.62.0
LLVM version: 14.0.5

@rustbot modify labels: +regression-from-stable-to-stable +O-windows

@smmalis37 smmalis37 added C-bug Category: This is a bug. regression-untriaged Untriaged performance or correctness regression. labels Jul 5, 2022
@rustbot rustbot added O-windows Operating system: Windows regression-from-stable-to-stable Performance or correctness regression from one stable version to another. I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Jul 5, 2022
@jstarks
Copy link

jstarks commented Jul 5, 2022

@ChrisDenton, I believe this must be because git bash (cygwin? msys?) is opening pipes between processes with FILE_FLAG_OVERLAPPED.

In my original bug report, I suggested calling GetOverlappedResult once if NtReadFile returned STATUS_PENDING, since that will at least resolve the pending IO correctly in the case where the file is not being used for other IOs concurrently. This would cover this and other cases at least as well as the original code calling ReadFile. This isn't perfect, since of course File::read takes &self, so you can probably deadlock in a multi-threaded process in this async pipe configuration. But it would at least avoid the common case.

However, it seems that this got missed from the actual change (sorry, I should have caught that). Is there a reason this was skipped?

@ChrisDenton
Copy link
Member

Oh, hrm. That looks like an oversight on my part. I'll PR a fix asap.

@apiraino apiraino added T-libs Relevant to the library team, which will review and decide on the PR/issue. and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Jul 6, 2022
@t1m0thyj
Copy link

t1m0thyj commented Jul 6, 2022

Thanks for fixing the issue quickly. 😀 When the PR that fixes this issue is merged, will a patch release 1.62.1 be published? 🙏 Or will we have to wait for a fix in 1.63?

@workingjubilee
Copy link
Member

workingjubilee commented Jul 6, 2022

PR #98950 was nominated and accepted for both beta and stable backport by T-libs.
Leaving this open until those play out.
Assigning P-high for the same reasons it was accepted for backports.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. O-windows Operating system: Windows P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants