View Issue Details

IDProjectCategoryView StatusLast Update
0000513LDMud 3.3Runtimepublic2018-01-29 21:57
Reporterzesstra Assigned Tolars 
PrioritynormalSeverityminorReproducibilityalways
Status resolvedResolutionfixed 
Product Version3.3 
Fixed in Version3.3.716 
Summary0000513: Errormessage from errorf() may be leaked
DescriptionRecently we had a garbage collector run, which revealed several memory leaks. I was able to associate one of these leaks to errorf(), which seems to leak the error message unter some very specific circumstances (see below for instructions).

2007.09.10 22:46:42 --- Garbage Collection ---
2007.09.10 22:46:43 GC pass 1: Freed 1 objects.
freeing small block 0x04092440 (user 0x04092448) simulate.c 801
  By object: secure/error
  By program: secure/error.c line:3
0409245c: 02 00 00 00 00 00 00 00 06 00 00 00 00 00 2a 65 ..............*e
0409246c: 67 61 6c 0a 00 0d 03 00 11 06 00 38 ab d8 58 ab gal........8..X.
0409247c: 74 d4 5a 05 15 00 00 t.Z....

freeing small block 0x03041ecc (user 0x03041ed4) simulate.c 801
  By object: players/zesstra/test
  By program: players/zesstra/test.c line:37
03041ee8: 02 00 00 00 00 00 00 00 0f 00 00 00 00 00 2a 77 ..............*w
03041ef8: 69 72 6b 6c 69 63 68 20 65 67 61 6c 0a 00 00 01 irklich egal....
03041f08: b2 08 00 38 08 3c 09 71 00 5a 2d 04 bd 05 00 ...8.<.q.Z-....

2 small blocks freed
2007.09.10 22:46:43 GC freed 0 destructed objects.

The leaked blocks were allocated by
  string_t * str = new_mstring(emsg_buf);

Steps To ReproduceOk, please consider the following objects:
/players/zesstra/test.c:
mixed test() {
  catch(raise_error("wirklich egal\n");publish);
}

/secure/error.c:
mixed LogError() {
  raise_error("egal\n");
}

/secure/master.c:
protected void runtime_error(...) {
...
    catch(call_other("/secure/error","LogError",
      err,prg,curobj,line,culprit,caught);publish);
...
}

Now call /players/zesstra/test->test() and initiate the garbage collector run. The result in the garbage collector log should be something like the memory dump given above.
Please note, that the error during the catch() in runtime_error() is important, without it there is no leak.
TagsNo tags attached.
Attached Files
catchleak.diff (6,670 bytes)   
Index: trunk/src/interpret.c
===================================================================
--- trunk/src/interpret.c	(Revision 2314)
+++ trunk/src/interpret.c	(Arbeitskopie)
@@ -269,6 +269,11 @@
     svalue_t             * save_sp;
       /* The saved global values
        */
+
+    svalue_t catch_value;
+      /* Holds the value throw()n from within a catch() while the throw
+       * is executed.
+       */
 };
 
 /* --- struct cache: one entry of the apply cache
@@ -569,12 +574,7 @@
    * is the real bottom of the stack.
    */
 
-svalue_t catch_value = { T_INVALID } ;
-  /* Holds the value throw()n from within a catch() while the throw
-   * is executed.
-   */
 
-
 #if MAX_USER_TRACE >= MAX_TRACE
 #error MAX_USER_TRACE value must be smaller than MAX_TRACE!
 #endif
@@ -6699,6 +6699,7 @@
     p->recovery_info.rt.last = rt_context;
     p->recovery_info.rt.type = ERROR_RECOVERY_CATCH;
     p->recovery_info.flags = catch_flags;
+    p->catch_value.type = T_INVALID;
     rt_context = (rt_context_t *)&p->recovery_info;
     return &p->recovery_info.con;
 } /* push_error_context() */
@@ -6733,7 +6734,7 @@
 
 /*-------------------------------------------------------------------------*/
 svalue_t *
-pull_error_context (svalue_t *sp)
+pull_error_context (svalue_t *sp, svalue_t *msg)
 
 /* Restore the context saved by a catch() after a throw() or runtime error
  * occured. <sp> is the current stackpointer and is used to pop the elements
@@ -6742,6 +6743,8 @@
  * The function pops the topmost recovery entry, which must be the catch
  * recovery entry, restores the important global variables and returns
  * the saved stack pointer.
+ *
+ * If <msg> is not NULL the caught error message is put there.
  */
 
 {
@@ -6780,6 +6783,12 @@
     csp = p->save_csp;
     pop_n_elems(sp - p->save_sp);
     command_giver = p->save_command_giver;
+    
+    /* Save the error message */
+    if (msg)
+        transfer_svalue_no_free(msg, &p->catch_value);
+    else
+        free_svalue(&p->catch_value);
 
     /* Remove the context from the context stack */
     rt_context = p->recovery_info.rt.last;
@@ -6790,6 +6799,19 @@
 
 /*-------------------------------------------------------------------------*/
 void
+transfer_error_message (svalue_t *v, rt_context_t *rt)
+ /* Saves the message <v> in the error context <rt> assuming that
+  * it's a catch recovery context. <v> is freed afterwards.
+  */
+{
+    struct catch_context *p;
+
+    p = (struct catch_context *)rt;
+    transfer_svalue_no_free(&p->catch_value, v);
+}
+
+/*-------------------------------------------------------------------------*/
+void
 push_control_stack ( svalue_t   *sp
                    , bytecode_p  pc
                    , svalue_t   *fp
@@ -15955,10 +15977,9 @@
          */
 
         assign_eval_cost();
-        transfer_svalue_no_free(&catch_value, sp--);
-        inter_sp = sp;
+        inter_sp = --sp;
         inter_pc = pc;
-        throw_error(); /* do the longjump, with extra checks... */
+        throw_error(sp+1); /* do the longjump, with extra checks... */
         break;
 
     /* --- Efuns: Strings --- */
Index: trunk/src/interpret.h
===================================================================
--- trunk/src/interpret.h	(Revision 2314)
+++ trunk/src/interpret.h	(Arbeitskopie)
@@ -108,7 +108,6 @@
 extern int32  eval_cost;
 extern int32  assigned_eval_cost;
 extern svalue_t apply_return_value;
-extern svalue_t catch_value;
 extern svalue_t last_indexing_protector;
 
 #ifdef APPLY_CACHE_STAT
@@ -138,7 +137,8 @@
 extern void pop_control_stack(void);
 extern struct longjump_s *push_error_context(svalue_t *sp, int catch_flags);
 extern void pop_error_context (void);
-extern svalue_t *pull_error_context (svalue_t *sp);
+extern svalue_t *pull_error_context (svalue_t *sp, svalue_t *msg);
+extern void transfer_error_message (svalue_t *v, rt_context_t *rt);
 extern Bool destructed_object_ref (svalue_t *svp);
 extern void free_object_svalue(svalue_t *v);
 extern void zero_object_svalue(svalue_t *v);
Index: trunk/src/simulate.c
===================================================================
--- trunk/src/simulate.c	(Revision 2314)
+++ trunk/src/simulate.c	(Arbeitskopie)
@@ -392,9 +392,10 @@
          * the global <catch_value>.
          */
         svalue_t *sp;
+        svalue_t catch_value;
 
         /* Remove the catch context and get the old stackpointer setting */
-        sp = pull_error_context(INTER_SP);
+        sp = pull_error_context(INTER_SP, &catch_value);
 
         /* beware of errors after set_this_object() */
         current_object = csp->ob;
@@ -407,7 +408,6 @@
 
         /* Push the catch return value */
         *(++sp) = catch_value;
-        catch_value.type = T_INVALID;
 
         *i_sp = (volatile svalue_t *)sp;
 
@@ -801,7 +801,12 @@
             string_t * str = new_mstring(emsg_buf);
 
             if (NULL != str)
-                put_string(&catch_value, str);
+            {
+                svalue_t stmp;
+
+                put_string(&stmp, str);
+                transfer_error_message(&stmp, rt);
+            }
             else
             {
                 error_caught = MY_FALSE;
@@ -1325,21 +1330,21 @@
 
 /*-------------------------------------------------------------------------*/
 void
-throw_error()
+throw_error (svalue_t *v)
 
-/* The second part of the efun throw(): the caller stored the message
- * into catch_value, now our job is to do the proper longjmp.
+/* The efun throw(). We have to save the message <v> in the
+ * error context and then do the proper longjmp. <v> is freed.
  */
 
 {
     unroll_context_stack();
     if (rt_context->type >= ERROR_RECOVERY_CATCH)
     {
+        transfer_error_message(v, rt_context);
         longjmp(((struct error_recovery_info *)rt_context)->con.text, 1);
         fatal("Throw_error failed!");
     }
-    free_svalue(&catch_value);
-    catch_value.type = T_INVALID;
+    free_svalue(v);
     errorf("Throw with no catch.\n");
 } /* throw_error() */
 
Index: trunk/src/simulate.h
===================================================================
--- trunk/src/simulate.h	(Revision 2314)
+++ trunk/src/simulate.h	(Arbeitskopie)
@@ -278,7 +278,7 @@
 extern void warnf VARPROT((char *, ...), printf, 1, 2);
 extern void errorf VARPROT((const char *, ...), printf, 1, 2) NORETURN;
 extern void fatal VARPROT((const char *, ...), printf, 1, 2) NORETURN;
-extern void throw_error(void);
+extern void throw_error(svalue_t *v);
 extern char *limit_error_format(char *fixed_fmt, size_t fixed_fmt_len, const char *fmt);
 extern Bool legal_path(const char *path);
 extern Bool check_no_parentdirs(const char *path);
catchleak.diff (6,670 bytes)   

Activities

zesstra

2007-09-10 15:10

administrator   ~0000540

Hngl. For anybody missing the instructions how to reproduce the leak, please switch to the advanced view of the bug report, without it the report ist pretty much useless. ;-)

Gnomi

2007-09-14 17:44

manager   ~0000542

The problem is one global variable catch_value that is overwritten in the nested call to errorf(). In the example there are three writes to that variable (1x "*wirklich egal\n" and 2x "*egal\n", because LogError() invokes runtime_error twice), the innermost catch() reads the variable and sets it back to T_INVALID, both outer catch()es get then T_INVALID as their result. Two of these three strings are leaked by then.

I uploaded a patch that should fix this issue. I put this variable catch_value into the catch_context, so each catch() gets its own instance of it.

Greetings,
Gnomi.

lars

2007-10-07 16:50

reporter   ~0000563

I implemented the patch as given. Thanks!

Issue History

Date Modified Username Field Change
2007-09-10 15:06 zesstra New Issue
2007-09-10 15:10 zesstra Note Added: 0000540
2007-09-14 17:36 Gnomi File Added: catchleak.diff
2007-09-14 17:44 Gnomi Note Added: 0000542
2007-10-07 16:50 lars Status new => resolved
2007-10-07 16:50 lars Fixed in Version => 3.3.716
2007-10-07 16:50 lars Resolution open => fixed
2007-10-07 16:50 lars Assigned To => lars
2007-10-07 16:50 lars Note Added: 0000563
2010-11-16 09:42 lars Source_changeset_attached => ldmud.git master 8da9441f
2018-01-29 18:59 lars Source_changeset_attached => ldmud.git master 8da9441f
2018-01-29 21:57 lars Source_changeset_attached => ldmud.git master 8da9441f