Bug #19753: IO::Buffer#get_string can't handle negative offset

• Status: Closed
• Assignee: ioquatix (Samuel Williams)
• Target version: -
• ruby -v: 3.2
• Backport: 3.0: DONTNEED, 3.1: REQUIRED, 3.2: DONE

[noteflakes (Sharon Rosner)]

irb(main):001:0> b = IO::Buffer.for('abc')
=> 
#<IO::Buffer 0x00007f858f5450c0+3 EXTERNAL READONLY SLICE>
...
irb(main):002:0> b.get_string(-1)
=> "\x00abc"
irb(main):003:0> b.get_string(-1000, 3)
(irb):3:in `get_string': Specified offset+length exceeds data size! (ArgumentError)
	from (irb):3:in `<main>'
	from /home/sharon/.rbenv/versions/3.2.0/lib/ruby/gems/3.2.0/gems/irb-1.7.1/exe/irb:9:in `<top (required)>'
	from /home/sharon/.rbenv/versions/3.2.0/bin/irb:25:in `load'
	from /home/sharon/.rbenv/versions/3.2.0/bin/irb:25:in `<main>'

Using a negative offset returns garbage in the string but it also might segfault:

irb(main):003:0> b = IO::Buffer.map(File.open('sgt-nodes.sql', 'r+'))
=> #<IO::Buffer 0x00007f189de14000+2008858 EXTERNAL MAPPED SHARED>
irb(main):004:0> b.get_string(-1000)
(irb):4: [BUG] Segmentation fault at 0x00007f189de13c18
ruby 3.2.0 (2022-12-25 revision a528908271) [x86_64-linux]

-- Control frame information -----------------------------------------------
c:0021 p:---- s:0109 e:000108 CFUNC  :get_string
...

Expected behaviour

I think it might be nice to have #get_string behave like other methods taking an offset, like String#[], so a negative offset means counting backwards from the end of the buffer. For example:

irb(main):001:0> b = IO::Buffer.for('abc')
=> 
#<IO::Buffer 0x00007f858f5450c0+3 EXTERNAL READONLY SLICE>
...
irb(main):002:0> b.get_string(-1)
=> "c"
irb(main):003:0> b.get_string(-2)
=> "bc"
irb(main):003:0> b.get_string(-1000)
=> "abc"
irb(main):003:0> b.get_string(-1000, 2)
=> "ab"

[noteflakes (Sharon Rosner)]

• Description updated (diff)

[noteflakes (Sharon Rosner)]

PR here: https://github.com/ruby/ruby/pull/8015

[nobu (Nobuyoshi Nakada)]

• Status changed from Open to Assigned
• Assignee set to ioquatix (Samuel Williams)
• Backport changed from 3.0: UNKNOWN, 3.1: UNKNOWN, 3.2: UNKNOWN to 3.0: DONTNEED, 3.1: REQUIRED, 3.2: REQUIRED

noteflakes (Sharon Rosner) wrote:

irb(main):003:0> b.get_string(-1000)
=> "abc"

I think this should raise an ArgumentError.

irb(main):003:0> b.get_string(-1000, 2)
=> "ab"

At least, it should not be "ab".

[noteflakes (Sharon Rosner)]

nobu (Nobuyoshi Nakada) wrote in #note-3:

irb(main):003:0> b.get_string(-1000)
=> "abc"

I think this should raise an ArgumentError.

irb(main):003:0> b.get_string(-1000, 2)
=> "ab"

At least, it should not be "ab".

Well now that I look at String#[] and Array#[] they return nil when offset < -size:

irb(main):001:0> s = 'abc'
=> "abc"
irb(main):002:0> s[-3]
=> "a"
irb(main):003:0> s[-4]
=> nil
irb(main):004:0> a = [1,2,3]
=> [1, 2, 3]
irb(main):005:0> a[-4]
=> nil
irb(main):006:0> a[-4, 2]
=> nil

Would a similar behaviour be acceptable for IO::Buffer?

[jeremyevans0 (Jeremy Evans)]

• Status changed from Assigned to Closed

This appears to be fixed by a095740fed2a05a04806a1d3827bcaba02e45720, as you now get: Offset can't be negative! (ArgumentError) for b.get_string(-1)

[ioquatix (Samuel Williams)]

@jeremyevans0 (Jeremy Evans) Thanks for reviewing the fix, yes, this should be closed.

[nagachika (Tomoyuki Chikanaga)]

https://github.com/nagachika/ruby/commit/7d325d1b2ea7b6ef7debd6a29ad97c93311ed9b5 Here is the changeset to backport into ruby_3_2 branch, including style change and refactoring (data -> buffer).
I have resolved some conflict by hand.
@ioquatix (Samuel Williams) Would you kindly review the backport commit?

[nagachika (Tomoyuki Chikanaga)]

I have cherry-picked some additional refactoring commits and open pull request https://github.com/ruby/ruby/pull/10778.

[ioquatix (Samuel Williams)]

Thanks so much, I will review it.

[nagachika (Tomoyuki Chikanaga)]

• Backport changed from 3.0: DONTNEED, 3.1: REQUIRED, 3.2: REQUIRED to 3.0: DONTNEED, 3.1: REQUIRED, 3.2: DONE

Backported by https://github.com/ruby/ruby/pull/10778