* ext/syck/rubyext.c: fixed GC problem (backported HEAD 1.55 - 1.62)

  [ruby-dev:27839]

* ext/syck/syck.h (S_FREE): small hack. no need to check if pointer is
  NULL or not before S_FREE.

* st.c: uses malloc instead of xmalloc to avoid GC. syck uses st_insert
  in gram.c to insert node from rb_syck_bad_anchor_handler into
  SyckParser's hash table. if GC occurs in st_insert, it's not under
  SyckParser's mark system yet. so RString can be released wrongly.
  [ruby-dev:28057]


git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/branches/ruby_1_8@9722 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

2 files changed, 76 insertions, 101 deletions

diff --git a/ext/syck/rubyext.c b/ext/syck/rubyext.c
index 027211539b..272cfd2571 100644
--- a/ext/syck/rubyext.c
+++ b/ext/syck/rubyext.c
@@ -75,7 +75,7 @@ void rb_syck_err_handler _((SyckParser *, char *));
 SyckNode * rb_syck_bad_anchor_handler _((SyckParser *, char *));
 void rb_syck_output_handler _((SyckEmitter *, char *, long));
 void rb_syck_emitter_handler _((SyckEmitter *, st_data_t));
-int syck_parser_assign_io _((SyckParser *, VALUE));
+int syck_parser_assign_io _((SyckParser *, VALUE *));
 VALUE syck_scalar_alloc _((VALUE class));
 VALUE syck_seq_alloc _((VALUE class));
 VALUE syck_map_alloc _((VALUE class));
@@ -107,7 +107,7 @@ rb_syck_compile(self, port)
     bytestring_t *sav; 
 
     SyckParser *parser = syck_new_parser();
-    taint = syck_parser_assign_io(parser, port);
+    taint = syck_parser_assign_io(parser, &port);
     syck_parser_handler( parser, syck_yaml2byte_handler );
     syck_parser_error_handler( parser, NULL );
     syck_parser_implicit_typing( parser, 0 );
@@ -122,7 +122,7 @@ rb_syck_compile(self, port)
 
     syck_free_parser( parser );
 
-    bc = rb_str_new2( ret );
+    bc = rb_str_new2( ret );  S_FREE( ret );
     if ( taint )      OBJ_TAINT( bc );
     return bc;
 }
@@ -164,12 +164,12 @@ rb_syck_io_str_read( char *buf, SyckIoStr *str, long max_size, long skip )
  * (returns tainted? boolean)
  */
 int
-syck_parser_assign_io(parser, port)
+syck_parser_assign_io(parser, pport)
     SyckParser *parser;
-    VALUE port;
+    VALUE *pport;
 {
     int taint = Qtrue;
-    VALUE tmp;
+    VALUE tmp, port = *pport;
     if (!NIL_P(tmp = rb_check_string_type(port))) {
         taint = OBJ_TAINTED(port); /* original taintedness */
         port = tmp;
@@ -184,6 +184,7 @@ syck_parser_assign_io(parser, port)
     else {
         rb_raise(rb_eTypeError, "instance of IO needed");
     }
+    *pport = port;
     return taint;
 }
 
@@ -265,12 +266,11 @@ rb_syck_mktime(str, len)
     ptr += 2;
     if ( len > ptr - str && *ptr == '.' )
     {
-        char *padded = syck_strndup( "000000", 6 );
+        char padded[] = "000000";
         char *end = ptr + 1;
         while ( isdigit( *end ) ) end++;
         MEMCPY(padded, ptr + 1, char, end - (ptr + 1));
         usec = strtol(padded, NULL, 10);
-        S_FREE(padded);
     }
     else
     {
@@ -595,6 +595,8 @@ yaml_org_handler( n, ref )
     return transferred;
 }
 
+static void syck_node_mark( SyckNode *n );
+
 /*
  * {native mode} node handler
  * - Converts data into native Ruby types
@@ -703,6 +705,13 @@ syck_set_model( p, input, model )
     syck_parser_bad_anchor_handler( parser, rb_syck_bad_anchor_handler );
 }
 
+static int
+syck_st_mark_nodes( char *key, SyckNode *n, char *arg )
+{
+    if ( n != (void *)1 ) syck_node_mark( n );
+    return ST_CONTINUE;
+}
+
 /*
  * mark parser nodes
  */
@@ -710,14 +719,20 @@ static void
 syck_mark_parser(parser)
     SyckParser *parser;
 {
-    struct parser_xtra *bonus;
-    rb_gc_mark(parser->root);
-    rb_gc_mark(parser->root_on_error);
-    if ( parser->bonus != NULL )
+    struct parser_xtra *bonus = (struct parser_xtra *)parser->bonus;
+    rb_gc_mark_maybe(parser->root);
+    rb_gc_mark_maybe(parser->root_on_error);
+    rb_gc_mark( bonus->data );
+    rb_gc_mark( bonus->proc );
+    rb_gc_mark( bonus->resolver );
+
+    if ( parser->anchors != NULL )
     {
-        bonus = (struct parser_xtra *)parser->bonus;
-        rb_gc_mark( bonus->data );
-        rb_gc_mark( bonus->proc );
+        st_foreach( parser->anchors, syck_st_mark_nodes, 0 );
+    }
+    if ( parser->bad_anchors != NULL )
+    {
+        st_foreach( parser->bad_anchors, syck_st_mark_nodes, 0 );
     }
 }
 
@@ -728,8 +743,7 @@ void
 rb_syck_free_parser(p)
     SyckParser *p;
 {
-    struct parser_xtra *bonus = (struct parser_xtra *)p->bonus;
-    if ( bonus != NULL ) S_FREE( bonus );
+    S_FREE( p->bonus );
     syck_free_parser(p);
 }
 
@@ -744,6 +758,9 @@ syck_parser_s_alloc(class)
     VALUE pobj;
     SyckParser *parser = syck_new_parser();
 
+    parser->bonus = S_ALLOC( struct parser_xtra );
+    S_MEMZERO( parser->bonus, struct parser_xtra, 1 );
+
     pobj = Data_Wrap_Struct( class, syck_mark_parser, rb_syck_free_parser, parser );
 
     syck_parser_set_root_on_error( parser, Qnil );
@@ -815,8 +832,7 @@ syck_parser_load(argc, argv, self)
 {
     VALUE port, proc, model, input;
     SyckParser *parser;
-    struct parser_xtra *bonus = S_ALLOC_N( struct parser_xtra, 1 );
-    volatile VALUE hash;    /* protect from GC */
+    struct parser_xtra *bonus;
 
     rb_scan_args(argc, argv, "11", &port, &proc);
 
@@ -825,13 +841,12 @@ syck_parser_load(argc, argv, self)
     Data_Get_Struct(self, SyckParser, parser);
     syck_set_model( self, input, model );
 
-    bonus->taint = syck_parser_assign_io(parser, port);
-    bonus->data = hash = rb_hash_new();
+    bonus = (struct parser_xtra *)parser->bonus;
+    bonus->taint = syck_parser_assign_io(parser, &port);
+    bonus->data = rb_hash_new();
     bonus->resolver = rb_attr_get( self, s_resolver );
     if ( NIL_P( proc ) ) bonus->proc = 0;
     else                 bonus->proc = proc;
-    
-    parser->bonus = (void *)bonus;
 
     return syck_parse( parser );
 }
@@ -847,8 +862,7 @@ syck_parser_load_documents(argc, argv, self)
 {
     VALUE port, proc, v, input, model;
     SyckParser *parser;
-    struct parser_xtra *bonus = S_ALLOC_N( struct parser_xtra, 1 );
-    volatile VALUE hash;
+    struct parser_xtra *bonus;
 
     rb_scan_args(argc, argv, "1&", &port, &proc);
 
@@ -857,15 +871,15 @@ syck_parser_load_documents(argc, argv, self)
     Data_Get_Struct(self, SyckParser, parser);
     syck_set_model( self, input, model );
     
-    bonus->taint = syck_parser_assign_io(parser, port);
+    bonus = (struct parser_xtra *)parser->bonus;
+    bonus->taint = syck_parser_assign_io(parser, &port);
     bonus->resolver = rb_attr_get( self, s_resolver );
     bonus->proc = 0;
-    parser->bonus = (void *)bonus;
 
     while ( 1 )
     {
         /* Reset hash for tracking nodes */
-        bonus->data = hash = rb_hash_new();
+        bonus->data = rb_hash_new();
 
         /* Parse a document */
         v = syck_parse( parser );
@@ -1187,10 +1201,9 @@ syck_resolver_tagurize( self, val )
 
     if ( !NIL_P(tmp) )
     {
-        char *taguri;
-        val = tmp;
-        taguri = syck_type_id_to_uri( RSTRING(val)->ptr );
-        return rb_str_new2( taguri );
+        char *taguri = syck_type_id_to_uri( RSTRING(tmp)->ptr );
+        val = rb_str_new2( taguri );
+        S_FREE( taguri );
     }
 
     return val;
@@ -1380,6 +1393,7 @@ syck_node_mark( n )
     SyckNode *n;
 {
     int i;
+    rb_gc_mark_maybe( n->id );
     switch ( n->kind )
     {
         case syck_seq_kind:
@@ -1397,42 +1411,9 @@ syck_node_mark( n )
             }
         break;
     }
-}
-
-/*
- * Don't free Ruby data, Ruby will do that
- */
-void
-rb_syck_free_node( SyckNode *n )
-{
-    switch ( n->kind  )
-    {
-        case syck_str_kind:
-            S_FREE( n->data.str );
-            n->data.str = NULL;
-        break;
-
-        case syck_seq_kind:
-            if ( n->data.list != NULL )
-            {
-                S_FREE( n->data.list->items );
-                S_FREE( n->data.list );
-                n->data.list = NULL;
-            }
-        break;
-
-        case syck_map_kind:
-            if ( n->data.pairs != NULL )
-            {
-                S_FREE( n->data.pairs->keys );
-                S_FREE( n->data.pairs->values );
-                S_FREE( n->data.pairs );
-                n->data.pairs = NULL;
-            }
-        break;
-    }
-
-    S_FREE( n );
+#if 0 /* maybe needed */
+    if ( n->shortcut ) syck_node_mark( n->shortcut ); /* caution: maybe cyclic */
+#endif
 }
 
 /*
@@ -1443,7 +1424,7 @@ syck_scalar_alloc( class )
     VALUE class;
 {
     SyckNode *node = syck_alloc_str();
-    VALUE obj = Data_Wrap_Struct( class, syck_node_mark, rb_syck_free_node, node );
+    VALUE obj = Data_Wrap_Struct( class, syck_node_mark, syck_free_node, node );
     node->id = obj;
     return obj;
 }
@@ -1512,7 +1493,7 @@ syck_scalar_value_set( self, val )
     Data_Get_Struct( self, SyckNode, node );
 
     StringValue( val );
-    node->data.str->ptr = RSTRING(val)->ptr;
+    node->data.str->ptr = syck_strndup( RSTRING(val)->ptr, RSTRING(val)->len );
     node->data.str->len = RSTRING(val)->len;
     node->data.str->style = scalar_none;
 
@@ -1530,7 +1511,7 @@ syck_seq_alloc( class )
     SyckNode *node;
     VALUE obj;
     node = syck_alloc_seq();
-    obj = Data_Wrap_Struct( class, syck_node_mark, rb_syck_free_node, node );
+    obj = Data_Wrap_Struct( class, syck_node_mark, syck_free_node, node );
     node->id = obj;
     return obj;
 }
@@ -1629,7 +1610,7 @@ syck_map_alloc( class )
     SyckNode *node;
     VALUE obj;
     node = syck_alloc_map();
-    obj = Data_Wrap_Struct( class, syck_node_mark, rb_syck_free_node, node );
+    obj = Data_Wrap_Struct( class, syck_node_mark, syck_free_node, node );
     node->id = obj;
     return obj;
 }
@@ -1759,8 +1740,7 @@ syck_node_init_copy( copy, orig )
     if ( copy == orig )
         return copy;
 
-    if ( TYPE( orig ) != T_DATA ||
-         RDATA( orig )->dfree != ( RUBY_DATA_FUNC )rb_syck_free_node )
+    if ( TYPE( orig ) != T_DATA )
     {
         rb_raise( rb_eTypeError, "wrong argument type" );
     }
@@ -1781,12 +1761,11 @@ syck_node_type_id_set( self, type_id )
     SyckNode *node;
     Data_Get_Struct( self, SyckNode, node );
 
-    if ( node->type_id != NULL ) S_FREE( node->type_id );
+    S_FREE( node->type_id );
 
-    if ( NIL_P( type_id ) ) {
-        node->type_id = NULL;
-    } else {
-        node->type_id = StringValuePtr( type_id );
+    if ( !NIL_P( type_id ) ) {
+        StringValue( type_id );
+        node->type_id = syck_strndup( RSTRING(type_id)->ptr, RSTRING(type_id)->len );
     }
 
     rb_iv_set( self, "@type_id", type_id );
@@ -1804,13 +1783,14 @@ syck_node_transform( self )
     SyckNode *n;
     SyckNode *orig_n;
     Data_Get_Struct(self, SyckNode, orig_n);
+    t = Data_Wrap_Struct( cNode, syck_node_mark, syck_free_node, 0 );
 
     switch (orig_n->kind)
     {
         case syck_map_kind:
             {
                 int i;
-                n = syck_alloc_map();
+                DATA_PTR(t) = n = syck_alloc_map();
                 for ( i = 0; i < orig_n->data.pairs->idx; i++ )
                 {
                     syck_map_add( n, rb_funcall( syck_map_read( orig_n, map_key, i ), s_transform, 0 ),
@@ -1822,7 +1802,7 @@ syck_node_transform( self )
         case syck_seq_kind:
             {
                 int i;
-                n = syck_alloc_seq();
+                DATA_PTR(t) = n = syck_alloc_seq();
                 for ( i = 0; i < orig_n->data.list->idx; i++ )
                 {
                     syck_seq_add( n, rb_funcall( syck_seq_read( orig_n, i ), s_transform, 0 ) );
@@ -1831,7 +1811,7 @@ syck_node_transform( self )
         break;
 
         case syck_str_kind:
-            n = syck_new_str2( orig_n->data.str->ptr, orig_n->data.str->len, orig_n->data.str->style );
+            DATA_PTR(t) = n = syck_new_str2( orig_n->data.str->ptr, orig_n->data.str->len, orig_n->data.str->style );
         break;
     }
 
@@ -1843,11 +1823,8 @@ syck_node_transform( self )
     {
         n->anchor = syck_strndup( orig_n->anchor, strlen( orig_n->anchor ) );
     }
-    t = Data_Wrap_Struct( cNode, NULL, NULL, n );
     n->id = t;
-    t = rb_funcall( oDefaultResolver, s_node_import, 1, t );
-    syck_free_node( n );
-    return t;
+    return rb_funcall( oDefaultResolver, s_node_import, 1, t );
 }
 
 /*
@@ -1942,13 +1919,10 @@ static void
 syck_mark_emitter(emitter)
     SyckEmitter *emitter;
 {
-    struct emitter_xtra *bonus;
-    if ( emitter->bonus != NULL )
-    {
-        bonus = (struct emitter_xtra *)emitter->bonus;
-        rb_gc_mark( bonus->data );
-        rb_gc_mark( bonus->port );
-    }
+    struct emitter_xtra *bonus = (struct emitter_xtra *)emitter->bonus;
+    rb_gc_mark( bonus->oid  );
+    rb_gc_mark( bonus->data );
+    rb_gc_mark( bonus->port );
 }
 
 /*
@@ -1958,8 +1932,7 @@ void
 rb_syck_free_emitter(e)
     SyckEmitter *e;
 {
-    struct emitter_xtra *bonus = (struct emitter_xtra *)e->bonus;
-    if ( bonus != NULL ) S_FREE( bonus );
+    S_FREE( e->bonus );
     syck_free_emitter(e);
 }
 
@@ -1974,6 +1947,9 @@ syck_emitter_s_alloc(class)
     VALUE pobj;
     SyckEmitter *emitter = syck_new_emitter();
 
+    emitter->bonus = S_ALLOC( struct emitter_xtra );
+    S_MEMZERO( emitter->bonus, struct emitter_xtra, 1 );
+
     pobj = Data_Wrap_Struct( class, syck_mark_emitter, rb_syck_free_emitter, emitter );
     syck_emitter_handler( emitter, rb_syck_emitter_handler );
     syck_output_handler( emitter, rb_syck_output_handler );
@@ -1994,15 +1970,13 @@ syck_emitter_reset( argc, argv, self )
     VALUE options, tmp;
     SyckEmitter *emitter;
     struct emitter_xtra *bonus;
-    volatile VALUE hash;    /* protect from GC */
 
     Data_Get_Struct(self, SyckEmitter, emitter);
     bonus = (struct emitter_xtra *)emitter->bonus;
-    if ( bonus != NULL ) S_FREE( bonus );
 
-    bonus = S_ALLOC_N( struct emitter_xtra, 1 );
+    bonus->oid = Qnil;
     bonus->port = rb_str_new2( "" );
-    bonus->data = hash = rb_hash_new();
+    bonus->data = rb_hash_new();
 
     if (rb_scan_args(argc, argv, "01", &options) == 0)
     {
@@ -2024,7 +1998,6 @@ syck_emitter_reset( argc, argv, self )
     }
     
     emitter->headless = 0;
-    emitter->bonus = (void *)bonus;
     rb_ivar_set(self, s_level, INT2FIX(0));
     rb_ivar_set(self, s_resolver, Qnil);
     return self;
@@ -2252,10 +2225,12 @@ Init_syck()
     rb_define_method( cResolver, "node_import", syck_resolver_node_import, 1 );
     rb_define_method( cResolver, "tagurize", syck_resolver_tagurize, 1 );
 
+    rb_global_variable( &oDefaultResolver );
     oDefaultResolver = rb_funcall( cResolver, rb_intern( "new" ), 0 );
     rb_define_singleton_method( oDefaultResolver, "node_import", syck_defaultresolver_node_import, 1 );
     rb_define_singleton_method( oDefaultResolver, "detect_implicit", syck_defaultresolver_detect_implicit, 1 );
     rb_define_const( rb_syck, "DefaultResolver", oDefaultResolver );
+    rb_global_variable( &oGenericResolver );
     oGenericResolver = rb_funcall( cResolver, rb_intern( "new" ), 0 );
     rb_define_singleton_method( oGenericResolver, "node_import", syck_genericresolver_node_import, 1 );
     rb_define_const( rb_syck, "GenericResolver", oGenericResolver );
diff --git a/ext/syck/syck.h b/ext/syck/syck.h
index f7ac39fc94..c49f740173 100644
--- a/ext/syck/syck.h
+++ b/ext/syck/syck.h
@@ -51,7 +51,7 @@ extern "C" {
 #define S_ALLOC_N(type,n) (type*)malloc(sizeof(type)*(n))
 #define S_ALLOC(type) (type*)malloc(sizeof(type))
 #define S_REALLOC_N(var,type,n) (var)=(type*)realloc((char*)(var),sizeof(type)*(n))
-#define S_FREE(n) free(n); n = NULL;
+#define S_FREE(n) if (n) { free(n); n = NULL; }
 
 #define S_ALLOCA_N(type,n) (type*)alloca(sizeof(type)*(n))