Skip to content

Commit

Permalink
Fix possible segfaults after GC.compact
Browse files Browse the repository at this point in the history
This adds several rb_gc_mark calls for VALUEs that were only implicit marked before.
This is because ruby-2.7 adds a second meaning to rb_gc_mark().
It not only marks a VALUE as in use, but also marks the VALUE as unmoveable, so that it's pinned to one memory address.

In ruby < 2.7 is was sufficient to mark objects that otherwise would freed by GC.
So there was no need to mark objects that were implicit in use, like self references.
Strating with ruby-2.7 these objects must be marked as well.
Alternatively C extentions can use compatation callbacks to update VALUE references.

I verified that this patch is effective to fix these issues by placing several instances of the following line into specs:
  GC.verify_compaction_references(toward: :empty, double_heap: true)

Unfortunately GC.verify_compaction_references is not suitable to be added regulary to the specs, because every instance of this call doubles the process memory in use.

See also GC.compact discussion: https://bugs.ruby-lang.org/issues/15626#Known-Issue

Fixes #327
Fixes #328
  • Loading branch information
larskanis committed Feb 1, 2020
1 parent 093741a commit 7c1756f
Show file tree
Hide file tree
Showing 5 changed files with 24 additions and 5 deletions.
1 change: 1 addition & 0 deletions ext/pg.h
Original file line number Diff line number Diff line change
Expand Up @@ -304,6 +304,7 @@ VALUE pg_obj_to_i _(( VALUE ));
VALUE pg_tmbc_allocate _(( void ));
void pg_coder_init_encoder _(( VALUE ));
void pg_coder_init_decoder _(( VALUE ));
void pg_coder_mark _(( t_pg_coder * ));
char *pg_rb_str_ensure_capa _(( VALUE, long, char *, char ** ));

#define PG_RB_STR_ENSURE_CAPA( str, expand_len, curr_ptr, end_ptr ) \
Expand Down
20 changes: 16 additions & 4 deletions ext/pg_coder.c
Original file line number Diff line number Diff line change
Expand Up @@ -61,11 +61,23 @@ pg_coder_init_decoder( VALUE self )
rb_iv_set( self, "@name", Qnil );
}

void
pg_coder_mark(t_pg_coder *this)
{
rb_gc_mark(this->coder_obj);
}

static void
pg_composite_coder_mark(t_pg_composite_coder *this)
{
pg_coder_mark(&this->comp);
}

static VALUE
pg_simple_encoder_allocate( VALUE klass )
{
t_pg_coder *this;
VALUE self = Data_Make_Struct( klass, t_pg_coder, NULL, -1, this );
VALUE self = Data_Make_Struct( klass, t_pg_coder, pg_coder_mark, -1, this );
pg_coder_init_encoder( self );
return self;
}
Expand All @@ -74,7 +86,7 @@ static VALUE
pg_composite_encoder_allocate( VALUE klass )
{
t_pg_composite_coder *this;
VALUE self = Data_Make_Struct( klass, t_pg_composite_coder, NULL, -1, this );
VALUE self = Data_Make_Struct( klass, t_pg_composite_coder, pg_composite_coder_mark, -1, this );
pg_coder_init_encoder( self );
this->elem = NULL;
this->needs_quotation = 1;
Expand All @@ -87,7 +99,7 @@ static VALUE
pg_simple_decoder_allocate( VALUE klass )
{
t_pg_coder *this;
VALUE self = Data_Make_Struct( klass, t_pg_coder, NULL, -1, this );
VALUE self = Data_Make_Struct( klass, t_pg_coder, pg_coder_mark, -1, this );
pg_coder_init_decoder( self );
return self;
}
Expand All @@ -96,7 +108,7 @@ static VALUE
pg_composite_decoder_allocate( VALUE klass )
{
t_pg_composite_coder *this;
VALUE self = Data_Make_Struct( klass, t_pg_composite_coder, NULL, -1, this );
VALUE self = Data_Make_Struct( klass, t_pg_composite_coder, pg_composite_coder_mark, -1, this );
pg_coder_init_decoder( self );
this->elem = NULL;
this->needs_quotation = 1;
Expand Down
1 change: 1 addition & 0 deletions ext/pg_copy_coder.c
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ typedef struct {
static void
pg_copycoder_mark( t_pg_copycoder *this )
{
pg_coder_mark(&this->comp);
rb_gc_mark(this->typemap);
rb_gc_mark(this->null_string);
}
Expand Down
1 change: 1 addition & 0 deletions ext/pg_record_coder.c
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ typedef struct {
static void
pg_recordcoder_mark( t_pg_recordcoder *this )
{
pg_coder_mark(&this->comp);
rb_gc_mark(this->typemap);
}

Expand Down
6 changes: 5 additions & 1 deletion ext/pg_type_map_by_class.c
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,11 @@ pg_tmbk_mark( t_tmbk *this )
{
rb_gc_mark(this->typemap.default_typemap);
rb_gc_mark(this->klass_to_coder);
/* All coders are in the Hash, so no need to mark the cache. */
rb_gc_mark(this->self);
/* Clear the cache, to be safe from changes of klass VALUE by GC.compact.
* TODO: Move cache clearing to compactation callback provided by Ruby-2.7+.
*/
memset(&this->cache_row, 0, sizeof(this->cache_row));
}

static VALUE
Expand Down

0 comments on commit 7c1756f

Please sign in to comment.