Add checks and specs for writing to inline array and char array fields #756
Conversation
|
|
||
| it 'raises when writing a longer String to the char array' do | ||
| expect { @s[:str] = "abcdefg" }.to raise_error(IndexError) | ||
| end |
There was a problem hiding this comment.
Do you agree these 2 specs should be the correct behavior?
Current output on MRI:
1) FFI::Struct with a char array field allows writing a String of the same size to the char array
Failure/Error: expect(@s[:after]).to eq(-1) # The above should not write to the next field
expected: -1
got: -256
(compared using ==)
# ./spec/ffi/struct_spec.rb:839:in `block (2 levels) in <module:StructSpecs>'
2) FFI::Struct with a char array field raises when writing a longer String to the char array
Failure/Error: expect { @s[:str] = "abcdefg" }.to raise_error(IndexError)
expected IndexError but nothing was raised
# ./spec/ffi/struct_spec.rb:843:in `block (2 levels) in <module:StructSpecs>'
There was a problem hiding this comment.
I implemented that behavior in the C extension.
eregon
force-pushed
the
char-array-specs
branch
from
a2fda9d
to
981fe12
Compare
eregon
force-pushed
the
char-array-specs
branch
from
981fe12
to
d8c21ea
Compare
|
Ready for review. |
|
I'll rebase this to get the CI fixes. |
eregon
force-pushed
the
char-array-specs
branch
from
d8c21ea
to
50eb96c
Compare
* Raise IndexError if the written String is longer than the char[]. * Don't write a final \0 if we write as many characters as the char[] length.
* Note that the last 2 specs used to fail before the previous commit.
eregon
changed the title
|
@larskanis Could you review this when you get some time? |
- Verify that the string length is counted in bytes (not characters) - Use negative values for :before and :after to notice overflows better - Specify explicit how zero char is handled
There was a problem hiding this comment.
Thank you @eregon to fill this specification gap. I did some small adjustments to the numbers used for testing in the last commit and agree with your change.
Strictly speaking this is an API change, but I think the spirit of FFI has always been to avoid any overflows, where possible. So I think we can release this with an increment of the FFI minor version number.
Adding CHANGELOG entries as part of the commit tend to result in unnecessary merge conflicts. So I usually write the CHANGELOG just before the release and use this to sort it and pimp it up for better readability. |
|
Thanks, your added commit looks good. Agreed, I think next minor version is fine, it would be a bug to rely on previous behavior (could overwrite unintended memory or lead to SEGV). I'll let you merge the PR. |
Part of #660 (comment)