View Issue Details

IDProjectCategoryView StatusLast Update
0000548LDMud 3.3Efunspublic2018-01-29 22:57
ReporterGnomi Assigned ToGnomi  
PrioritynormalSeveritymajorReproducibilityalways
Status resolvedResolutionfixed 
Platformi686OSDebian GNU/LinuxOS Version4.0
Product Version3.3.716 
Fixed in Version3.3.719 
Summary0000548: restore_value is not reentrant
DescriptionI tried to call restore_value recursively (via privilege_violation e.g. for error handling) and it crashed:

(restore) Freeing lost shared_restored_values.
2008.07.03 12:31:12 Null pointer to assign_svalue().
2008.07.03 12:31:12 Current object was unpriv

Program terminated with signal 8, Arithmetic exception.
#0 0x080fef36 in dump_core () at simulate.c:587
587 *((char*)0) = 0/a;
(gdb) bt
#0 0x080fef36 in dump_core () at simulate.c:587
0000001 0x080fecf9 in fatal (
    fmt=0x81438f8 "remove_from_free_list: block %p, magic match failed: expected %lx, found %lx\n") at simulate.c:609
0000002 0x0811b3be in remove_from_free_list (ptr=0x8cb3bcc) at slaballoc.c:2307
0000003 0x0811c5d3 in large_malloc (size=67, force_more=0) at slaballoc.c:3306
0000004 0x0811a212 in mem_alloc (size=254) at slaballoc.c:1574
0000005 0x0811d9c6 in xalloc_traced (size=254,
    malloc_trace_file=0x8141830 "strfuns.c", malloc_trace_line=123)
    at xalloc.c:540
0000006 0x081097d5 in strbuf_grow (buf=0xbfce90ac, len=78) at strfuns.c:123
0000007 0x081096c1 in strbuf_add (buf=0xbfce90ac,
    text=0xbfce7f78 "' epilog' in '", ' ' <repeats 12 times>, "master.c' ('", ' ' <repeats 14 times>, "master') line 21\n") at strfuns.c:152
0000008 0x08109a0f in strbuf_addf (buf=0xbfce90ac,
    format=0x812ccd0 "'%15s' in '%20s' ('%20s') line %d\n") at strfuns.c:226
0000009 0x080b2070 in collect_trace (sbuf=0xbfce90ac, rvec=0x0)
    at interpret.c:18878
0000010 0x080b250e in dump_trace (how=1, rvec=0x0) at interpret.c:18950
0000011 0x080fee46 in fatal (fmt=0x8129598 "Null pointer to assign_svalue().\n")
    at simulate.c:632
0000012 0x0808ebd4 in inl_assign_svalue_no_free (to=0x8c58fec, from=0x0)
    at interpret.c:1464
0000013 0x0808ebba in assign_svalue_no_free (to=0x8c58fec, from=0x0)
    at interpret.c:1522
#14 0x080daac2 in restore_svalue (svp=0x8c58fec, pt=0xbfce931c,
    delimiter=44 ',') at object.c:8432
#15 0x080db707 in restore_array (svp=0x8c58fec, str=0xbfce931c)
    at object.c:7600
#16 0x080da6eb in restore_svalue (svp=0x8168178, pt=0xbfce931c,
    delimiter=10 '\n') at object.c:8299
#17 0x080dcc2d in f_restore_value (sp=0x8168178) at object.c:9095
#18 0x08094686 in eval_instruction (first_instruction=0x8cb3ba2 "\n",
    initial_sp=0x8168168) at interpret.c:8003
#19 0x080ae1f1 in apply_low (fun=0x8c77440, ob=0x8cb3bd4, num_arg=0,
    b_ign_prot=0, allowRefs=0) at interpret.c:16843
#20 0x080ae3e8 in int_apply (fun=0x8c77440, ob=0x8cb3bd4, num_arg=0,
    b_ign_prot=0, b_use_default=1) at interpret.c:16921
#21 0x080a92c4 in eval_instruction (first_instruction=0x8cb06e2 "an\a",
    initial_sp=0x8168148) at interpret.c:16191
#22 0x080ae1f1 in apply_low (fun=0x8c09b1c, ob=0x8cb07a4, num_arg=1,
    b_ign_prot=1, allowRefs=0) at interpret.c:16843
#23 0x080ae3e8 in int_apply (fun=0x8c09b1c, ob=0x8cb07a4, num_arg=1,
    b_ign_prot=1, b_use_default=0) at interpret.c:16921
#24 0x080ae852 in sapply_int (fun=0x8c09b1c, ob=0x8cb07a4, num_arg=1,
    b_find_static=1, b_use_default=0) at interpret.c:17082
#25 0x080af0c5 in apply_master_ob (fun=0x8c09b1c, num_arg=1, external=1)
    at interpret.c:17376
#26 0x08055cd8 in preload_objects (eflag=1) at backend.c:1252
#27 0x080c083c in main (argc=40, argv=0xbfcebc74) at main.c:612
Steps To ReproduceI made a testcase and will add the testsuite to svn soon.
TagsNo tags attached.

Activities

Gnomi

2008-12-26 20:44

manager   ~0000832

I uploaded a patch that puts all global variables for restore_object/value into one structure that can be stacked (the error handlers take care of them). save_object/value might have the same problem, but I can't see how they can be called recursively.

2008-12-26 20:55

 

restore_value.diff (12,832 bytes)   
Index: trunk/test/t-0000548/master.c
===================================================================
--- trunk/test/t-0000548/master.c	(Revision 2455)
+++ trunk/test/t-0000548/master.c	(Arbeitskopie)
@@ -1,6 +1,7 @@
 #define OWN_PRIVILEGE_VIOLATION
 #include "/inc/base.inc"
 #include "/inc/sefun.inc"
+#include "/inc/gc.inc"
 
 void run_test()
 {
@@ -19,5 +20,6 @@
 string *epilog(int eflag)
 {
     run_test();
+    start_gc(#'shutdown);
     return 0;
 }
Index: trunk/src/object.c
===================================================================
--- trunk/src/object.c	(Revision 2455)
+++ trunk/src/object.c	(Arbeitskopie)
@@ -5536,23 +5536,31 @@
    * writing a savefile.
    */
 
-static int restored_host = -1;
-  /* Type of the host which wrote the savefile being restored.
-   */
+struct restore_context_s {
 
-static long current_shared_restored;
-  /* ID of the shared value currently restored
-   */
+    int restored_host;
+      /* Type of the host which wrote the savefile being restored.
+       */
 
-static svalue_t *shared_restored_values = NULL;
-  /* Array of restored shared values, so that later references
-   * can do a simple lookup by ID-1 (IDs start at 1).
-   */
+    long current_shared_restored;
+      /* ID of the shared value currently restored
+       */
 
-static long max_shared_restored;
-  /* Current size of shared_restored_values.
-   */
+    svalue_t *shared_restored_values;
+      /* Array of restored shared values, so that later references
+       * can do a simple lookup by ID-1 (IDs start at 1).
+       */
 
+    long max_shared_restored;
+      /* Current size of shared_restored_values.
+       */
+       
+    struct restore_context_s *previous;
+      /* The previous context. */
+};
+
+static struct restore_context_s *restore_ctx = NULL;
+
 /*-------------------------------------------------------------------------*/
 /* Macros */
 
@@ -7327,10 +7335,12 @@
  */
 
 {
-    while (current_shared_restored > 0)
-        free_svalue(&shared_restored_values[--current_shared_restored]);
-    xfree(shared_restored_values);
-    shared_restored_values = NULL;
+    struct restore_context_s *ctx = restore_ctx;
+    
+    while (ctx->current_shared_restored > 0)
+        free_svalue(&(ctx->shared_restored_values[--ctx->current_shared_restored]));
+    xfree(ctx->shared_restored_values);
+    ctx->shared_restored_values = NULL;
 }
 
 /*-------------------------------------------------------------------------*/
@@ -8373,7 +8383,7 @@
          * Otherwise, parse the float normally.
          */
         svp->type = T_FLOAT;
-        if ( NULL != (cp = strchr(cp, '=')) &&  restored_host == CURRENT_HOST)
+        if ( NULL != (cp = strchr(cp, '=')) &&  restore_ctx->restored_host == CURRENT_HOST)
         {
             cp++;
             if (sscanf(cp, "%"SCNxPHINT":%"SCNxPINT, &svp->x.exponent, &svp->u.mantissa) != 2)
@@ -8416,47 +8426,47 @@
             /* Shared values can be used even before they have been read in
              * completely.
              */
-            if (id != ++current_shared_restored)
+            if (id != ++(restore_ctx->current_shared_restored))
             {
-                current_shared_restored--;
+                restore_ctx->current_shared_restored--;
                 *svp = const0;
                 return MY_FALSE;
             }
 
             /* Increase shared_restored_values[] if necessary */
 
-            if (id > max_shared_restored)
+            if (id > restore_ctx->max_shared_restored)
             {
                 svalue_t *new;
 
-                max_shared_restored *= 2;
-                new = rexalloc(shared_restored_values
-                              , sizeof(svalue_t)*max_shared_restored
+                restore_ctx->max_shared_restored *= 2;
+                new = rexalloc(restore_ctx->shared_restored_values
+                              , sizeof(svalue_t)*(restore_ctx->max_shared_restored)
                               );
                 if (!new)
                 {
-                    current_shared_restored--;
+                    restore_ctx->current_shared_restored--;
                     *svp = const0;
                     errorf("(restore) Out of memory (%lu bytes) for "
                           "%ld shared values.\n"
-                          , (unsigned long)max_shared_restored * sizeof(svalue_t)
-                          , max_shared_restored);
+                          , (unsigned long)restore_ctx->max_shared_restored * sizeof(svalue_t)
+                          , restore_ctx->max_shared_restored);
                     return MY_FALSE;
                 }
-                shared_restored_values = new;
+                restore_ctx->shared_restored_values = new;
             }
 
             /* in case of an error... */
             *svp = const0;
-            shared_restored_values[id-1] = const0;
+            restore_ctx->shared_restored_values[id-1] = const0;
 
             /* Restore the value */
-            res = restore_svalue(&shared_restored_values[id-1], pt, delimiter);
-            assign_svalue_no_free(svp, &shared_restored_values[id-1]);
+            res = restore_svalue(&(restore_ctx->shared_restored_values[id-1]), pt, delimiter);
+            assign_svalue_no_free(svp, &(restore_ctx->shared_restored_values[id-1]));
             return res;
         }
 
-        if (id <= 0 || id > current_shared_restored)
+        if (id <= 0 || id > restore_ctx->current_shared_restored)
         {
             *svp = const0;
             return MY_FALSE;
@@ -8464,7 +8474,7 @@
 
         /* We know this value already: simply assign it */
 
-        assign_svalue_no_free(svp, &shared_restored_values[id-1]);
+        assign_svalue_no_free(svp, &(restore_ctx->shared_restored_values[id-1]));
 
         cp = strchr(cp, delimiter);
         *pt = cp+1;
@@ -8560,6 +8570,7 @@
 
 {
     restore_cleanup_t * data = (restore_cleanup_t *)arg;
+    struct restore_context_s * ctx;
 
     while (data->dp)
     {
@@ -8585,6 +8596,10 @@
         xfree(data->filename);
   
     xfree(arg);
+    
+    ctx = restore_ctx;
+    restore_ctx = ctx->previous;
+    xfree(ctx);    
 } /* restore_object_cleanup() */
 
 /*-------------------------------------------------------------------------*/
@@ -8640,6 +8655,8 @@
        */
     restore_cleanup_t * rcp;
       /* Cleanup structure */
+    struct restore_context_s * ctx;
+      /* Our helper structure. */
     
     arg = sp;
     
@@ -8648,18 +8665,35 @@
     rcp = xalloc(sizeof(*rcp));
     if (!rcp)
     {
-      errorf("(restore) Out of memory: (%zu bytes) for cleanup structure\n"
-             , sizeof(*rcp));
-      /* NOTREACHED */
-      return sp;
+        errorf("(restore) Out of memory: (%zu bytes) for cleanup structure\n"
+               , sizeof(*rcp));
+        /* NOTREACHED */
+        return sp;
     }
+    ctx = xalloc(sizeof(*ctx));
+    if (!ctx)
+    {
+        xfree(rcp);
+        errorf("(restore) Out of memory: (%zu bytes) for context structure\n"
+               , sizeof(*ctx));
+        /* NOTREACHED */
+        return sp;
+    }
+
     rcp->pNesting = &nesting;
     rcp->buff = NULL;
     rcp->f = NULL;
     rcp->dp = NULL;
     rcp->filename = NULL;
+
+    ctx->restored_host = -1;
+    ctx->current_shared_restored = 0;
+    ctx->shared_restored_values = NULL;
+    ctx->previous = restore_ctx;
+    
     /* Push it on top of the argument on the stack. */
     sp = push_error_handler(restore_object_cleanup, &(rcp->head));
+    restore_ctx = ctx;
   
     /* Keep track of recursive calls */
     nesting++;
@@ -8788,21 +8822,13 @@
 
     /* Initialise the variables */
 
-    max_shared_restored = 64;
-    current_shared_restored = 0;
+    ctx->max_shared_restored = 64;
+    ctx->shared_restored_values = xalloc(sizeof(svalue_t)*(ctx->max_shared_restored));
 
-    if (shared_restored_values)
+    if (!ctx->shared_restored_values)
     {
-        debug_message("(restore) Freeing lost shared_restored_values.\n");
-        free_shared_restored_values();
-    }
-
-    shared_restored_values = xalloc(sizeof(svalue_t)*max_shared_restored);
-
-    if (!shared_restored_values)
-    {
         errorf("(restore) Out of memory (%lu bytes) for shared values.\n"
-             , sizeof(svalue_t)*(unsigned long)max_shared_restored);
+             , sizeof(svalue_t)*(unsigned long)ctx->max_shared_restored);
         /* NOTREACHED */
         return sp;
     }
@@ -8810,7 +8836,7 @@
     num_var = ob->prog->num_variables;
     var_rest = 0;
     restored_version = -1;
-    restored_host = -1;
+    ctx->restored_host = -1;
 
     /* Loop until we run out of text to parse */
 
@@ -8854,7 +8880,7 @@
             {
                 int i;
 
-                i = sscanf(cur+1, "%d:%d", &restored_version, &restored_host);
+                i = sscanf(cur+1, "%d:%d", &restored_version, &(ctx->restored_host));
                 if (i > 0 && (i == 2 || restored_version >= CURRENT_VERSION) )
                 {
                     if (pt)
@@ -9016,6 +9042,7 @@
 
 {
     restore_cleanup_t * data = (restore_cleanup_t *) arg;
+    struct restore_context_s * ctx;
 
     if (data->buff)
         xfree(data->buff);
@@ -9023,6 +9050,10 @@
     free_shared_restored_values();
 
     xfree(arg);
+
+    ctx = restore_ctx;
+    restore_ctx = ctx->previous;
+    xfree(ctx);    
 } /* restore_value_cleanup() */
 
 svalue_t *
@@ -9041,15 +9072,51 @@
     int        restored_version; /* Formatversion of the saved data */
     char      *buff;  /* The string to parse */
     char      *p;
+    svalue_t  *arg;   /* pointer to the argument on the stack - for convenience */
     restore_cleanup_t *rcp; /* Cleanup structure */
+    struct restore_context_s * ctx; /* Our helper structure. */
 
+    /* Place the result variable onto the stack */
+    arg = sp;
+    inter_sp = ++sp;
+    *sp = const0;
+
+    /* Setup the error cleanup */
+    rcp = xalloc(sizeof(*rcp));
+    if (!rcp)
+    {
+        errorf("(restore) Out of memory (%zu bytes).\n"
+              , sizeof(*rcp));
+        /* NOTREACHED */
+        return sp;
+    }
+    ctx = xalloc(sizeof(*ctx));
+    if (!ctx)
+    {
+        xfree(rcp);
+        errorf("(restore) Out of memory: (%zu bytes) for context structure\n"
+             , sizeof(*ctx));
+        /* NOTREACHED */
+        return sp;
+    }
+
+    rcp->buff = NULL;
+
+    ctx->restored_host = -1;
+    ctx->current_shared_restored = 0;
+    ctx->shared_restored_values = NULL;
+    ctx->previous = restore_ctx;
+    
+    push_error_handler(restore_value_cleanup, &(rcp->head));
+    restore_ctx = ctx;
+
     /* The restore routines will put \0s into the string, so we
      * need to make a copy of all but malloced strings.
      */
     {
         size_t len;
 
-        len = mstrsize(sp->u.str);
+        len = mstrsize(arg->u.str);
         buff = xalloc(len+1);
         if (!buff)
         {
@@ -9058,58 +9125,31 @@
             /* NOTREACHED */
             return sp;
         }
-        memcpy(buff, get_txt(sp->u.str), len);
+        memcpy(buff, get_txt(arg->u.str), len);
         buff[len] = '\0';
+
+        rcp->buff = buff;
     }
 
     restored_version = -1;
-    restored_host = -1;
 
     /* Initialise the shared value table */
 
-    max_shared_restored = 64;
-
-    if (shared_restored_values)
+    ctx->max_shared_restored = 64;
+    ctx->shared_restored_values = xalloc(sizeof(svalue_t)*(ctx->max_shared_restored));
+    if (!ctx->shared_restored_values)
     {
-        debug_message("(restore) Freeing lost shared_restored_values.\n");
-        free_shared_restored_values();
-    }
-
-    shared_restored_values = xalloc(sizeof(svalue_t)*max_shared_restored);
-    if (!shared_restored_values)
-    {
-        xfree(buff);
         errorf("(restore) Out of memory (%lu bytes) for shared values.\n"
-             , (unsigned long)max_shared_restored * sizeof(svalue_t));
+             , (unsigned long)ctx->max_shared_restored * sizeof(svalue_t));
         return sp; /* flow control hint */
     }
 
-    current_shared_restored = 0;
-
-    /* Place the result variable onto the stack */
-    inter_sp = ++sp;
-    *sp = const0;
-
-    /* Setup the error cleanup */
-    rcp = xalloc(sizeof(*rcp));
-    if (!rcp)
-    {
-        xfree(buff);
-        errorf("(restore) Out of memory (%zu bytes).\n"
-              , sizeof(*rcp));
-        /* NOTREACHED */
-        return sp;
-    }
-    rcp->buff = buff;
-
-    push_error_handler(restore_value_cleanup, &(rcp->head));
-
     /* Check if there is a version line */
     if (buff[0] == '#')
     {
         int i;
 
-        i = sscanf(buff+1, "%d:%d", &restored_version, &restored_host);
+        i = sscanf(buff+1, "%d:%d", &restored_version, &(ctx->restored_host));
 
         /* Advance to the next line */
         p = strchr(buff, '\n');
restore_value.diff (12,832 bytes)   

Gnomi

2009-01-14 09:20

manager   ~0000890

Committed as r2490.

Issue History

Date Modified Username Field Change
2008-07-03 06:37 Gnomi New Issue
2008-07-03 06:37 Gnomi Status new => assigned
2008-07-03 06:37 Gnomi Assigned To => Gnomi
2008-12-26 20:42 Gnomi File Added: restore_value.diff
2008-12-26 20:44 Gnomi Note Added: 0000832
2008-12-26 20:55 Gnomi File Deleted: restore_value.diff
2008-12-26 20:55 Gnomi File Added: restore_value.diff
2009-01-14 09:20 Gnomi Note Added: 0000890
2009-01-14 09:20 Gnomi Status assigned => resolved
2009-01-14 09:20 Gnomi Fixed in Version => 3.3.719
2009-01-14 09:20 Gnomi Resolution open => fixed
2010-11-16 10:42 Gnomi Source_changeset_attached => ldmud.git master 4101ca03
2018-01-29 19:59 Gnomi Source_changeset_attached => ldmud.git master 4101ca03
2018-01-29 22:57 Gnomi Source_changeset_attached => ldmud.git master 4101ca03