diff --git a/lib/net/protocol.rb b/lib/net/protocol.rb index a62a269..763ec6a 100644 --- a/lib/net/protocol.rb +++ b/lib/net/protocol.rb @@ -189,6 +189,7 @@ def close public def read(len, dest = ''.b, ignore_eof = false) + raise ArgumentError, "negative length #{len} given" if len < 0 LOG "reading #{len} bytes..." read_bytes = 0 begin @@ -356,9 +357,9 @@ def writing @debug_output << '<- ' if @debug_output yield @debug_output << "\n" if @debug_output - bytes = @written_bytes + @written_bytes + ensure @written_bytes = nil - bytes end def write0(*strs) @@ -384,6 +385,9 @@ def write0(*strs) # next string end # continue looping + when :wait_readable + (io = @io.to_io).wait_readable(@write_timeout) or raise Net::WriteTimeout.new(io) + # continue looping when :wait_writable (io = @io.to_io).wait_writable(@write_timeout) or raise Net::WriteTimeout.new(io) # continue looping @@ -426,12 +430,15 @@ def initialize(*, **) def each_message_chunk LOG 'reading message...' LOG_off() - read_bytes = 0 - while (line = readuntil("\r\n")) != ".\r\n" - read_bytes += line.size - yield line.delete_prefix('.') + begin + read_bytes = 0 + while (line = readuntil("\r\n")) != ".\r\n" + read_bytes += line.size + yield line.delete_prefix('.') + end + ensure + LOG_on() end - LOG_on() LOG "read message (#{read_bytes} bytes)" end @@ -457,12 +464,15 @@ def write_message_0(src) def write_message(src) LOG "writing message from #{src.class}" LOG_off() - len = writing { - using_each_crlf_line { - write_message_0 src + begin + len = writing { + using_each_crlf_line { + write_message_0 src + } } - } - LOG_on() + ensure + LOG_on() + end LOG "wrote #{len} bytes" len end @@ -470,16 +480,19 @@ def write_message(src) def write_message_by_block(&block) LOG 'writing message from block' LOG_off() - len = writing { - using_each_crlf_line { - begin - block.call(WriteAdapter.new(self.method(:write_message_0))) - rescue LocalJumpError - # allow `break' from writer block - end + begin + len = writing { + using_each_crlf_line { + begin + block.call(WriteAdapter.new(self.method(:write_message_0))) + rescue LocalJumpError + # allow `break' from writer block + end + } } - } - LOG_on() + ensure + LOG_on() + end LOG "wrote #{len} bytes" len end @@ -499,6 +512,8 @@ def using_each_crlf_line write0 "\r\n" end write0 ".\r\n" + nil + ensure @wbuf = nil end diff --git a/test/net/protocol/test_protocol.rb b/test/net/protocol/test_protocol.rb index 44df36f..a6820bf 100644 --- a/test/net/protocol/test_protocol.rb +++ b/test/net/protocol/test_protocol.rb @@ -310,4 +310,120 @@ def test_readuntil_ignore_eof_returns_what_is_left # https://github.com/ruby/net assert_equal "ab\r", io.readuntil("\r") assert_equal "\n\r\nc", io.readuntil("\r\n\r\n", true) end + + # The length reaches rbuf_consume, which walks @rbuf_offset backwards by + # it, so a negative one has to be rejected before the buffer moves. + def test_read_rejects_a_negative_length # https://github.com/ruby/net-protocol/pull/69 + io = Net::BufferedIO.new(StringIO.new("abcdef".dup)) + assert_equal "a", io.read(1) + e = assert_raise(ArgumentError) { io.read(-1) } + assert_equal "negative length -1 given", e.message + assert_equal "bcdef", io.read_all + end + + # OpenSSL::Buffering#write_nonblock documents :wait_readable, which a + # renegotiation produces. Takes the write only once the caller has + # waited, and caps the attempts so a regression fails instead of + # spinning until CI gives up. + class WaitReadableWriteIO + MAX_WRITES = 3 + + attr_reader :string, :waits + + def initialize(becomes_readable: true) + @becomes_readable = becomes_readable + @string = "".b + @writes = 0 + @waits = 0 + end + + def to_io; self; end + + def wait_readable(_timeout) + @waits += 1 + @becomes_readable + end + + def write_nonblock(str, exception: true) + @writes += 1 + raise "write0 ignored :wait_readable: #{@writes} attempts" if @writes > MAX_WRITES + return :wait_readable if @waits.zero? + @string << str + str.bytesize + end + end + + def test_write0_waits_for_readability # https://github.com/ruby/net-protocol/pull/70 + mockio = WaitReadableWriteIO.new + io = Net::BufferedIO.new(mockio) + io.write_timeout = 0.1 + assert_equal 5, io.write("hello") + assert_equal "hello", mockio.string + assert_equal 1, mockio.waits + end + + def test_write0_times_out_waiting_for_readability # https://github.com/ruby/net-protocol/pull/70 + mockio = WaitReadableWriteIO.new(becomes_readable: false) + io = Net::BufferedIO.new(mockio) + io.write_timeout = 0.1 + assert_raise(Net::WriteTimeout) { io.write("hello") } + assert_equal 1, mockio.waits + end + + def test_write_message_by_block # https://github.com/ruby/net-protocol/pull/71 + sio = StringIO.new("".dup) + imio = Net::InternetMessageIO.new(sio) + assert_equal 10, imio.write_message_by_block { |dest| dest.write("hello\r\n") } + assert_equal "hello\r\n.\r\n", sio.string + end + + # `break' leaves through write_message_by_block itself, so the message + # stays unterminated and the method answers nil. + def test_write_message_by_block_allows_break # https://github.com/ruby/net-protocol/pull/71 + sio = StringIO.new("".dup) + imio = Net::InternetMessageIO.new(sio) + assert_nil imio.write_message_by_block { |dest| dest.write("a\r\n"); break } + assert_equal "a\r\n", sio.string + end + + def test_write_message_by_block_restores_logging_when_the_block_raises # https://github.com/ruby/net-protocol/pull/71 + sio = StringIO.new("".dup) + imio = Net::InternetMessageIO.new(sio) + debug = "".dup + imio.debug_output = debug + + assert_raise(RuntimeError) do + imio.write_message_by_block { |dest| dest.write("partial"); raise "boom" } + end + + assert_same debug, imio.debug_output + # Nothing outside reaches the half-written line or the byte count, so + # they have to be read back from the inside. + assert_nil imio.instance_variable_get(:@wbuf) + assert_nil imio.instance_variable_get(:@written_bytes) + end + + def test_write_message_restores_logging_when_the_source_raises # https://github.com/ruby/net-protocol/pull/71 + sio = StringIO.new("".dup) + imio = Net::InternetMessageIO.new(sio) + debug = "".dup + imio.debug_output = debug + + src = Object.new + def src.each; yield "partial"; raise "boom"; end + + assert_raise(RuntimeError) { imio.write_message(src) } + + assert_same debug, imio.debug_output + end + + def test_each_message_chunk_restores_logging_when_the_block_raises # https://github.com/ruby/net-protocol/pull/71 + imio = Net::InternetMessageIO.new(StringIO.new("line\r\n.\r\n".dup)) + debug = "".dup + imio.debug_output = debug + + assert_raise(RuntimeError) { imio.each_message_chunk { raise "boom" } } + + assert_same debug, imio.debug_output + end end