View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0000513 | LDMud 3.3 | Runtime | public | 2007-09-10 15:06 | 2018-01-29 21:57 |
Reporter | zesstra | Assigned To | |||
Priority | normal | Severity | minor | Reproducibility | always |
Status | resolved | Resolution | fixed | ||
Product Version | 3.3 | ||||
Fixed in Version | 3.3.716 | ||||
Summary | 0000513: Errormessage from errorf() may be leaked | ||||
Description | Recently 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 Reproduce | Ok, 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. | ||||
Tags | No 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); | ||||
|
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. ;-) |
|
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. |
|
I implemented the patch as given. Thanks! |
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 |
|
Status | new => resolved |
2007-10-07 16:50 |
|
Fixed in Version | => 3.3.716 |
2007-10-07 16:50 |
|
Resolution | open => fixed |
2007-10-07 16:50 |
|
Assigned To | => lars |
2007-10-07 16:50 |
|
Note Added: 0000563 | |
2010-11-16 09:42 |
|
Source_changeset_attached | => ldmud.git master 8da9441f |
2018-01-29 18:59 |
|
Source_changeset_attached | => ldmud.git master 8da9441f |
2018-01-29 21:57 |
|
Source_changeset_attached | => ldmud.git master 8da9441f |