From 8e111bd0592caca17da0d958c1b6e13bf4b63885 Mon Sep 17 00:00:00 2001 From: qaisjp Date: Thu, 24 Jul 2025 10:57:42 -0400 Subject: [PATCH 1/8] Add test for empty string stdin bug MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This test demonstrates a bug in subprocess.communicate where passing an empty string ("") causes a "closed stream" error. The issue only happens with an empty string input, not with nil or non-empty strings. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude --- test/test_empty_stdin.rb | 48 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) create mode 100644 test/test_empty_stdin.rb diff --git a/test/test_empty_stdin.rb b/test/test_empty_stdin.rb new file mode 100644 index 0000000..9a99c6d --- /dev/null +++ b/test/test_empty_stdin.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true +require 'rubygems' +gem 'minitest' +require 'minitest/autorun' +require 'subprocess' + +# Test to confirm bug where subprocess.communicate("") doesn't properly close stdin +describe Subprocess do + describe "empty string stdin issue" do + it "should not cause a broken pipe with empty string input" do + # This script waits for stdin to close and then outputs "stdin closed" + script = <<-EOF + # Wait for stdin to close, then output a message + require 'io/wait' + STDIN.wait_readable + puts "stdin closed" + EOF + + # Run the process and communicate with empty string input + Subprocess.check_call(['ruby', '-e', script], + stdin: Subprocess::PIPE, + stdout: Subprocess::PIPE) do |p| + stdout, _ = p.communicate("") + assert_includes(stdout, "stdin closed", "STDIN was not properly closed with empty string input") + end + end + + it "should work fine with non-empty string input" do + # Same test but with non-empty input + script = <<-EOF + # Wait for stdin to close, then output a message + require 'io/wait' + content = STDIN.read + puts "received: \#{content}" + puts "stdin closed" + EOF + + # Run the process and communicate with non-empty string input + Subprocess.check_call(['ruby', '-e', script], + stdin: Subprocess::PIPE, + stdout: Subprocess::PIPE) do |p| + stdout, _ = p.communicate("boop") + assert_includes(stdout, "received: boop", "Input was not received properly") + assert_includes(stdout, "stdin closed", "STDIN was not properly closed with non-empty input") + end + end + end +end \ No newline at end of file From 01400eb788eab1e94a39b10cec6cff0983a85b7f Mon Sep 17 00:00:00 2001 From: qaisjp Date: Thu, 24 Jul 2025 10:58:51 -0400 Subject: [PATCH 2/8] Fix empty string stdin bug in communicate method MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, when passing an empty string to communicate(), stdin would be closed immediately but then later used in IO.select calls, causing a "closed stream" error. This fix: 1. Only closes stdin immediately for nil input 2. For empty string input, closes stdin after determining wait_w and removes it from the wait_w array to prevent IO.select from using it 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude --- lib/subprocess.rb | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/lib/subprocess.rb b/lib/subprocess.rb index fbf274c..6fc9cc5 100644 --- a/lib/subprocess.rb +++ b/lib/subprocess.rb @@ -436,13 +436,23 @@ def communicate(input=nil, timeout_s=nil) # the input depending on how many bytes were written input = input.dup.force_encoding('BINARY') unless input.nil? - @stdin.close if (input.nil? || input.empty?) && !@stdin.nil? + # Close stdin immediately only if input is nil + # For empty strings, we'll close it after adding it to wait_w + @stdin.close if input.nil? && !@stdin.nil? timeout_at = Time.now + timeout_s if timeout_s self.class.catching_sigchld(pid) do |global_read, self_read| wait_r = [@stdout, @stderr, self_read, global_read].compact wait_w = [input && @stdin].compact + + # For empty string input, close stdin immediately after determining wait_w + # This ensures stdin is properly closed but won't be used in IO.select + if !input.nil? && input.empty? && !@stdin.nil? + @stdin.close + wait_w = [] + end + done = false while !done # If the process has exited, we want to drain any remaining output before returning From 449a1040a6b5c024b5a93d7fa133e4c94bdad0bd Mon Sep 17 00:00:00 2001 From: qaisjp Date: Thu, 24 Jul 2025 11:08:33 -0400 Subject: [PATCH 3/8] Improve empty string stdin fix and tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Improvements made: 1. Cleaner logic in communicate() method - explicitly handle wait_w array setup 2. Better edge case handling - check for already closed stdin 3. Improved tests that directly reproduce the original bug condition 4. Added comprehensive test coverage for nil, empty string, and edge cases The fix now properly handles: - Empty string input: closes stdin immediately, doesn't add to IO.select - Nil input: closes stdin immediately (existing behavior) - Non-empty input: adds stdin to wait_w for writing (existing behavior) - Already closed stdin: gracefully handled without errors 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude --- lib/subprocess.rb | 20 ++++++++----- test/test_empty_stdin.rb | 65 +++++++++++++++++++++------------------- 2 files changed, 47 insertions(+), 38 deletions(-) diff --git a/lib/subprocess.rb b/lib/subprocess.rb index 6fc9cc5..76dced2 100644 --- a/lib/subprocess.rb +++ b/lib/subprocess.rb @@ -437,20 +437,24 @@ def communicate(input=nil, timeout_s=nil) input = input.dup.force_encoding('BINARY') unless input.nil? # Close stdin immediately only if input is nil - # For empty strings, we'll close it after adding it to wait_w - @stdin.close if input.nil? && !@stdin.nil? + # For empty strings, we'll close it in the wait_w setup + @stdin.close if input.nil? && !@stdin.nil? && !@stdin.closed? timeout_at = Time.now + timeout_s if timeout_s self.class.catching_sigchld(pid) do |global_read, self_read| wait_r = [@stdout, @stderr, self_read, global_read].compact - wait_w = [input && @stdin].compact - # For empty string input, close stdin immediately after determining wait_w - # This ensures stdin is properly closed but won't be used in IO.select - if !input.nil? && input.empty? && !@stdin.nil? - @stdin.close - wait_w = [] + # Set up write file descriptors for IO.select + wait_w = [] + if !input.nil? && !@stdin.nil? && !@stdin.closed? + if input.empty? + # For empty input, close stdin immediately and don't add to wait_w + @stdin.close + else + # For non-empty input, add stdin to wait_w for writing + wait_w = [@stdin] + end end done = false diff --git a/test/test_empty_stdin.rb b/test/test_empty_stdin.rb index 9a99c6d..b521923 100644 --- a/test/test_empty_stdin.rb +++ b/test/test_empty_stdin.rb @@ -4,45 +4,50 @@ require 'minitest/autorun' require 'subprocess' -# Test to confirm bug where subprocess.communicate("") doesn't properly close stdin describe Subprocess do - describe "empty string stdin issue" do - it "should not cause a broken pipe with empty string input" do - # This script waits for stdin to close and then outputs "stdin closed" - script = <<-EOF - # Wait for stdin to close, then output a message - require 'io/wait' - STDIN.wait_readable - puts "stdin closed" - EOF - - # Run the process and communicate with empty string input - Subprocess.check_call(['ruby', '-e', script], + describe "communicate with empty string input" do + # Bug report: subprocess.communicate("") doesn't properly + # close stdin, causing you to end up a broken pipe. + it "should not raise IOError when passing empty string" do + # Before the fix, this would raise: IOError: closed stream + Subprocess.check_call(['cat'], stdin: Subprocess::PIPE, stdout: Subprocess::PIPE) do |p| - stdout, _ = p.communicate("") - assert_includes(stdout, "stdin closed", "STDIN was not properly closed with empty string input") + stdout, stderr = p.communicate("") + assert_equal("", stdout, "Empty input should produce empty output") + assert_equal("", stderr, "No errors expected") end end - it "should work fine with non-empty string input" do - # Same test but with non-empty input - script = <<-EOF - # Wait for stdin to close, then output a message - require 'io/wait' - content = STDIN.read - puts "received: \#{content}" - puts "stdin closed" - EOF + it "should work correctly with non-empty string input" do + test_input = "hello world" + Subprocess.check_call(['cat'], + stdin: Subprocess::PIPE, + stdout: Subprocess::PIPE) do |p| + stdout, stderr = p.communicate(test_input) + assert_equal(test_input, stdout, "Input should be echoed back") + assert_equal("", stderr, "No errors expected") + end + end - # Run the process and communicate with non-empty string input - Subprocess.check_call(['ruby', '-e', script], + it "should work correctly with nil input" do + Subprocess.check_call(['cat'], stdin: Subprocess::PIPE, stdout: Subprocess::PIPE) do |p| - stdout, _ = p.communicate("boop") - assert_includes(stdout, "received: boop", "Input was not received properly") - assert_includes(stdout, "stdin closed", "STDIN was not properly closed with non-empty input") + stdout, stderr = p.communicate(nil) + assert_equal("", stdout, "Nil input should produce empty output") + assert_equal("", stderr, "No errors expected") end end + + it "should handle already closed stdin gracefully" do + # Edge case: what if stdin is already closed? + p = Subprocess.popen(['cat'], stdin: Subprocess::PIPE, stdout: Subprocess::PIPE) + p.stdin.close + stdout, stderr = p.communicate("") + assert_equal("", stdout) + assert_equal("", stderr) + p.wait + end end -end \ No newline at end of file +end From 5557fff4f0dcc084ab2c3b5c82ac3d44b2456302 Mon Sep 17 00:00:00 2001 From: qaisjp Date: Thu, 24 Jul 2025 11:33:15 -0400 Subject: [PATCH 4/8] Refactor stdin handling to use stdin_closed variable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Improves code readability by: - Using stdin_closed variable to track state instead of repeating checks - Maintaining clear separation between nil and empty string handling - Eliminating repetitive @stdin.closed? checks - Preserving original logic flow and behavior 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude --- lib/subprocess.rb | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/lib/subprocess.rb b/lib/subprocess.rb index 76dced2..27b12de 100644 --- a/lib/subprocess.rb +++ b/lib/subprocess.rb @@ -437,8 +437,11 @@ def communicate(input=nil, timeout_s=nil) input = input.dup.force_encoding('BINARY') unless input.nil? # Close stdin immediately only if input is nil - # For empty strings, we'll close it in the wait_w setup - @stdin.close if input.nil? && !@stdin.nil? && !@stdin.closed? + stdin_closed = false + if input.nil? && !@stdin.nil? && !@stdin.closed? + @stdin.close + stdin_closed = true + end timeout_at = Time.now + timeout_s if timeout_s @@ -447,7 +450,7 @@ def communicate(input=nil, timeout_s=nil) # Set up write file descriptors for IO.select wait_w = [] - if !input.nil? && !@stdin.nil? && !@stdin.closed? + if !input.nil? && !@stdin.nil? && !stdin_closed if input.empty? # For empty input, close stdin immediately and don't add to wait_w @stdin.close From bf65d6e98ef7b3712ffb3d2d798eada56f6ee1cd Mon Sep 17 00:00:00 2001 From: qaisjp Date: Thu, 24 Jul 2025 11:39:45 -0400 Subject: [PATCH 5/8] Simplify stdin handling with unified logic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Improvements: - Move empty string handling to top with nil handling - Use single stdin_closed variable as source of truth - Handle pre-closed stdin edge case properly - Simplify wait_w logic to just check \!stdin_closed This creates cleaner, more maintainable code with the same behavior. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude --- lib/subprocess.rb | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/lib/subprocess.rb b/lib/subprocess.rb index 27b12de..f25b1b7 100644 --- a/lib/subprocess.rb +++ b/lib/subprocess.rb @@ -436,11 +436,15 @@ def communicate(input=nil, timeout_s=nil) # the input depending on how many bytes were written input = input.dup.force_encoding('BINARY') unless input.nil? - # Close stdin immediately only if input is nil + # Close stdin immediately if input is nil or empty stdin_closed = false - if input.nil? && !@stdin.nil? && !@stdin.closed? - @stdin.close - stdin_closed = true + if !@stdin.nil? + if @stdin.closed? + stdin_closed = true + elsif input.nil? || input.empty? + @stdin.close + stdin_closed = true + end end timeout_at = Time.now + timeout_s if timeout_s @@ -450,14 +454,9 @@ def communicate(input=nil, timeout_s=nil) # Set up write file descriptors for IO.select wait_w = [] - if !input.nil? && !@stdin.nil? && !stdin_closed - if input.empty? - # For empty input, close stdin immediately and don't add to wait_w - @stdin.close - else - # For non-empty input, add stdin to wait_w for writing - wait_w = [@stdin] - end + if !stdin_closed && !@stdin.nil? + # Add stdin to wait_w for writing + wait_w = [@stdin] end done = false From 58683eea1361202545ca587155a4788503e7ec3a Mon Sep 17 00:00:00 2001 From: qaisjp Date: Thu, 24 Jul 2025 11:47:22 -0400 Subject: [PATCH 6/8] rewrite but tests fail --- lib/subprocess.rb | 20 +++----------------- 1 file changed, 3 insertions(+), 17 deletions(-) diff --git a/lib/subprocess.rb b/lib/subprocess.rb index f25b1b7..a665751 100644 --- a/lib/subprocess.rb +++ b/lib/subprocess.rb @@ -437,28 +437,14 @@ def communicate(input=nil, timeout_s=nil) input = input.dup.force_encoding('BINARY') unless input.nil? # Close stdin immediately if input is nil or empty - stdin_closed = false - if !@stdin.nil? - if @stdin.closed? - stdin_closed = true - elsif input.nil? || input.empty? - @stdin.close - stdin_closed = true - end - end + @stdin.close if @stdin && (input.nil? || input.empty?) timeout_at = Time.now + timeout_s if timeout_s self.class.catching_sigchld(pid) do |global_read, self_read| wait_r = [@stdout, @stderr, self_read, global_read].compact - - # Set up write file descriptors for IO.select - wait_w = [] - if !stdin_closed && !@stdin.nil? - # Add stdin to wait_w for writing - wait_w = [@stdin] - end - + wait_w = @stdin&.closed? ? [] : [input && @stdin] + done = false while !done # If the process has exited, we want to drain any remaining output before returning From 9cfe1fcd8da358839f1f0efcbe94501b9adf1088 Mon Sep 17 00:00:00 2001 From: qaisjp Date: Thu, 24 Jul 2025 11:57:36 -0400 Subject: [PATCH 7/8] rewrite --- lib/subprocess.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/subprocess.rb b/lib/subprocess.rb index a665751..aca7f05 100644 --- a/lib/subprocess.rb +++ b/lib/subprocess.rb @@ -443,7 +443,7 @@ def communicate(input=nil, timeout_s=nil) self.class.catching_sigchld(pid) do |global_read, self_read| wait_r = [@stdout, @stderr, self_read, global_read].compact - wait_w = @stdin&.closed? ? [] : [input && @stdin] + wait_w = @stdin&.closed? ? [] : [input && @stdin].compact done = false while !done From 642b66fd5243ce2581c17e26aa04a4ce2560d44b Mon Sep 17 00:00:00 2001 From: qaisjp Date: Thu, 24 Jul 2025 12:03:44 -0400 Subject: [PATCH 8/8] Tweak comments --- test/test_empty_stdin.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/test_empty_stdin.rb b/test/test_empty_stdin.rb index b521923..b555c8d 100644 --- a/test/test_empty_stdin.rb +++ b/test/test_empty_stdin.rb @@ -6,11 +6,11 @@ describe Subprocess do describe "communicate with empty string input" do - # Bug report: subprocess.communicate("") doesn't properly - # close stdin, causing you to end up a broken pipe. + # Bug report: subprocess.communicate("") doesn't properly handle stdin, + # causing it to close incorrectly and result in a broken pipe. it "should not raise IOError when passing empty string" do # Before the fix, this would raise: IOError: closed stream - Subprocess.check_call(['cat'], + Subprocess.check_call(['cat'], stdin: Subprocess::PIPE, stdout: Subprocess::PIPE) do |p| stdout, stderr = p.communicate("") @@ -21,7 +21,7 @@ it "should work correctly with non-empty string input" do test_input = "hello world" - Subprocess.check_call(['cat'], + Subprocess.check_call(['cat'], stdin: Subprocess::PIPE, stdout: Subprocess::PIPE) do |p| stdout, stderr = p.communicate(test_input) @@ -31,7 +31,7 @@ end it "should work correctly with nil input" do - Subprocess.check_call(['cat'], + Subprocess.check_call(['cat'], stdin: Subprocess::PIPE, stdout: Subprocess::PIPE) do |p| stdout, stderr = p.communicate(nil)