Skip to content

Conversation

@JAi-SATHVIK
Copy link
Contributor

Problem:-

The previous implementation attempted to redirect standard input by appending the shell operator < filename directly to the command string (e.g., cmd < input.txt).

The Problem: The Windows API function CreateProcess does not spawn a shell (like cmd.exe). Consequently, it does not recognize or interpret shell operators.

The Result: The < character and the filename were passed as literal arguments to the child program. This caused the child program to receive "garbage" arguments while the actual stdin stream remained unassigned.

Solution:-

Removed Incorrect Syntax: The code no longer appends < filename to the command line string.

Native Handle Management: Utilizes CreateFile to open a legitimate, read-only handle to the specified input file (or NUL if no file is provided).

Process Inheritance: The handle is assigned to the hStdInput field of the STARTUPINFO structure.

The STARTF_USESTDHANDLES flag is enabled to instruct the OS to use the provided handles.

Resource Cleanup: Proper logic was added to ensure the file handle is closed safely after the process is launched, preventing resource leaks.

@codecov
Copy link

codecov bot commented Dec 29, 2025

Codecov Report

❌ Patch coverage is 90.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 68.69%. Comparing base (af58730) to head (d2fdd50).
⚠️ Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
test/system/test_subprocess.f90 90.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1079      +/-   ##
==========================================
+ Coverage   68.60%   68.69%   +0.08%     
==========================================
  Files         393      393              
  Lines       12710    12720      +10     
  Branches     1377     1376       -1     
==========================================
+ Hits         8720     8738      +18     
+ Misses       3990     3982       -8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jalvesz
Copy link
Contributor

jalvesz commented Dec 30, 2025

Thanks for catching this @JAi-SATHVIK!

The fact that you catched such a flagrant bug when the CI didn't brings me to the conclusion that the tests in place for this functionality are not thorough enough. Would it be possible to add a unit test that would ensure the correct behavior of the API?

Copy link
Member

@perazz perazz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the catch! LGTM overall. Before merging, I suggest:

  • Make stdin redirection work in all cases: on Windows the child can only use hStdInput if the underlying handle is allowed to be “passed” to the child process. Please open both the real input file and the fallback NUL handle in a way that allows passing to the child, and ensure CreateProcess is called with handle passing enabled.
  • Test: add a small Windows test that redirects stdin from a file and verifies the child reads expected content.
  • Cleanup/leak: close any opened handles on all early-return/error paths to avoid leaks.

@JAi-SATHVIK JAi-SATHVIK requested a review from perazz January 1, 2026 09:34
@jvdp1
Copy link
Member

jvdp1 commented Jan 4, 2026

With 2 approvals, I will merge this PR.
Thank you @JAi-SATHVIK for fixing this bug.
Thank you @perazz and @jalvesz for your reviews.

@jvdp1 jvdp1 merged commit 17a320a into fortran-lang:master Jan 4, 2026
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants