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

RB_GC_GUARD to protect from premature GC #283

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@johnsyweb

johnsyweb commented Jan 5, 2016

Context

I have been auditing some third-party Gems for memory errors in our app, which I believe are caused by premature GC in C extensions.

Change

In each of these writer functions, self is passed by value but isn't referenced after we take a pointer to it (sw), so could be optimised away after that. Allocs (or reallocs) could cause a GC to clean up self, invalidating sw. I introduce RB_GC_GUARD(self); later in the function to prevent this.

RB_GC_GUARD to protect from premature GC
Context
-----------

I have been auditing some third-party Gems for memory errors in our app, which I believe are caused by [premature GC in C extensions](https://github.com/ruby/ruby/blob/trunk/doc/extension.rdoc#appendix-e-rb_gc_guard-to-protect-from-premature-gc).

Change
------------

In each of these writer functions, `self` is passed by value but isn't referenced after we take a pointer to it (`sw`), so could be optimised away after that. Allocs (or reallocs) could cause a GC to clean up `self`, invalidating `sw`. I introduce `RB_GC_GUARD(self);` later in the function to prevent this.
@johnsyweb

This comment has been minimized.

johnsyweb commented Jan 5, 2016

Ignore me.

@johnsyweb johnsyweb closed this Jan 5, 2016

@johnsyweb johnsyweb deleted the envato:RB_GC_GUARD branch Jan 5, 2016

@ohler55

This comment has been minimized.

Owner

ohler55 commented Jan 5, 2016

The self VALUE is on the Ruby call stack so will not be GCed. That is true of any argument on the Ruby call stack. If the object were allocated in the C function that would be a potential problem with being GCed early but that is not the case in the changes you made.

@ohler55

This comment has been minimized.

Owner

ohler55 commented Jan 5, 2016

Race condition. Commented at the same time. :-)

@johnsyweb

This comment has been minimized.

johnsyweb commented Jan 5, 2016

Thank you for reviewing this PR so quickly! :-)

@ohler55

This comment has been minimized.

Owner

ohler55 commented Jan 5, 2016

Just happened to be at my desk. Alway good to have people reviewing code. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment