Skip to content

Fix memory corruption with FFI backend #180

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 21, 2025

Conversation

ankane
Copy link
Contributor

@ankane ankane commented Apr 21, 2025

To reproduce with JRuby or TruffleRuby:

require "fiddle"

1000.times do
  Fiddle::Pointer[rand(255).chr*16]
end

put_string adds a null byte at the end vs write_bytes. This is not caught by the bounds check since the underlying FFI::Pointer size is not set.

Comment on lines 242 to 243
cptr = Pointer.malloc(value.bytesize + 1)
cptr.ffi_ptr.put_string(0, value)
Copy link
Member

Choose a reason for hiding this comment

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

We may need to use put_bytes instead...

Suggested change
cptr = Pointer.malloc(value.bytesize + 1)
cptr.ffi_ptr.put_string(0, value)
cptr = Pointer.malloc(value.bytesize)
cptr.ffi_ptr.put_bytes(0, value)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried that initially with write_bytes, but looks like some of the tests expect an additional null byte.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nevermind, looks like the failing tests are due to an issue with the to_s method.

Copy link
Member

Choose a reason for hiding this comment

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

@kou Is it expected though that Fiddle::Pointer.to_ptr("abc") doesn't guarantee to return a \0-terminated native string?

Copy link
Member

@eregon eregon Apr 24, 2025

Choose a reason for hiding this comment

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

This is what the C extension does:

fiddle/ext/fiddle/pointer.c

Lines 784 to 787 in 7a4c6da

else if (RTEST(rb_obj_is_kind_of(val, rb_cString))){
char *str = StringValuePtr(val);
wrap = val;
ptr = rb_fiddle_ptr_new(str, RSTRING_LEN(val), NULL);

StringValuePtr AFAIK always returns a \0 terminated char* in CRuby (same as RSTRING_PTR).
So then

cptr = Pointer.malloc(value.bytesize + 1)
cptr.ffi_ptr.put_string(0, value)

is I believe the correct way.

i.e. it was correct as of 07a606f

Copy link
Member

@eregon eregon Apr 24, 2025

Choose a reason for hiding this comment

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

@ankane Could you make a PR going back to that and also add a test that Fiddle::Pointer.to_ptr("abc")[3] is always 0?

Also maybe the Pointer#size should be checked to be 4 and not 3, and then

ptr = rb_fiddle_ptr_new(str, RSTRING_LEN(val), NULL);
adapted to RSTRING_LEN(val) + 1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made the behavior consistent with CRuby in #187.

@ankane ankane force-pushed the ffi-backend-corruption branch from 07a606f to ae9b0bb Compare April 21, 2025 01:56
@kou kou merged commit 6d33e91 into ruby:master Apr 21, 2025
@kou
Copy link
Member

kou commented Apr 21, 2025

Thanks.

@ankane
Copy link
Contributor Author

ankane commented Apr 21, 2025

Thank you

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.

3 participants