Skip to content
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

Disallow re-layout of existing Struct #734

Closed
headius opened this issue Jan 8, 2020 · 7 comments · Fixed by #735
Closed

Disallow re-layout of existing Struct #734

headius opened this issue Jan 8, 2020 · 7 comments · Fixed by #735

Comments

@headius
Copy link
Contributor

headius commented Jan 8, 2020

I ran into this while attempting to align all Ruby code in JRuby and the FFI gem.

Currently, it is possible to define a struct and change its layout repeatedly:

class PairLayout < FFI::Struct
  layout :a, :int, :b, :long_long
end
class PairLayout < FFI::Struct
  layout :a, :int, 0, :b, :long_long, 4 # allowed
end

However, this seems like not only an odd feature, but a bad/dangerous one. When the layout has been used for any existing objects, changing it will produce very difficult-to-diagnose bugs (at least) or potentially memory violations if the layout is re-retrieved from the class and used for previous pointers.

At some point, it appears this was disallowed but the error was commented out for some reason: https://github.com/ffi/ffi/blob/master/lib/ffi/struct.rb#L207

This behavior caused a failure on JRuby running struct_spec.rb, because two specs there open the same PairLayout struct and call layout each time. JRuby's implementation of FFI expects that after the first struct has been allocated the layout will not be changed again, and caches it to avoid repeatedly looking up the instance variable. This caching resulted in the first spec's layout being used for the second, causing it to fail.

I believe we should reinstate the error. I cannot think of any good reasons why you would want to re-layout a struct after you have already started creating instances of it, and many good reasons why doing so would be a bad idea.

For now, I have removed the caching in JRuby, but I'd like to revert that (and the C FFI could potentially start caching the layout then as well): jruby/jruby@21a8a8c

@headius
Copy link
Contributor Author

headius commented Jan 8, 2020

At the very least, I believe we should consider the behavior "undefined" and modify the specs to not re-layout the same struct class. I would prefer to have a hard error, since there's really no good reason to modify a struct's layout after it is already in flight.

@larskanis
Copy link
Member

I fully agree. The raise is just disable because the tests depend heavily on it. I'll enable the raise and fix the tests today.

larskanis added a commit to larskanis/ffi that referenced this issue Jan 8, 2020
This seems like not only an odd feature, but a bad/dangerous one.
When the layout has been used for any existing objects, changing it will produce
very difficult-to-diagnose bugs (at least) or potentially memory violations
if the layout is re-retrieved from the class and used for previous pointers.

It was heavily used in the specs to overwrite one and the same class multiple times to change the layout immediatelly before the test.
However this use case is specific to testing and should not be used anyway.

Fixes ffi#734
@larskanis
Copy link
Member

OK, I pushed the changes here: https://github.com/ffi/ffi/pull/735/files?w=1

@larskanis
Copy link
Member

Given that we had a lot of such redefinitions in our tests and the benchmarks, I'm somewhat hesitated to release #735 as ffi-1.12.0. But on the other hand it isn't really worth a ffi-2.0.0 release. Maybe we should only warn about a struct layout redefinition?

@headius
Copy link
Contributor Author

headius commented Jan 8, 2020

I would be fine with a deprecation warning indicating that this is unspecified behavior that will be prohibited in the future. Thanks for the quick response! I will import the updated tests.

@eregon
Copy link
Collaborator

eregon commented Jan 9, 2020

The change looks good to me. If it's only FFI tests/benchs then I think it's fine to go hard error. It would be good to maybe check a couple key gems using FFI to make sure they don't rely on that behavior.

@larskanis
Copy link
Member

@eregon I just checked the reverse dependencies of ffi and it shows 816 gems. That's why I want to change it to a warning.

larskanis added a commit to larskanis/ffi that referenced this issue Jan 9, 2020
This seems like not only an odd feature, but a bad/dangerous one.
When the layout has been used for any existing objects, changing it will produce
very difficult-to-diagnose bugs (at least) or potentially memory violations
if the layout is re-retrieved from the class and used for previous pointers.

It was heavily used in the specs to overwrite one and the same class multiple times to change the layout immediatelly before the test.
However this use case is specific to testing and should not be used anyway.

Fixes ffi#734
headius added a commit to headius/jruby that referenced this issue Jan 10, 2020
This reverts commit 21a8a8c.

Per ffi/ffi#734, the behavior that broke this caching is now
deprecated and will be removed in ffi 2.0.
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 a pull request may close this issue.

3 participants