View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0000548 | LDMud 3.3 | Efuns | public | 2008-07-03 04:37 | 2018-01-29 21:57 |
Reporter | Gnomi | Assigned To | Gnomi | ||
Priority | normal | Severity | major | Reproducibility | always |
Status | resolved | Resolution | fixed | ||
Platform | i686 | OS | Debian GNU/Linux | OS Version | 4.0 |
Product Version | 3.3.716 | ||||
Fixed in Version | 3.3.719 | ||||
Summary | 0000548: restore_value is not reentrant | ||||
Description | I 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 Reproduce | I made a testcase and will add the testsuite to svn soon. | ||||
Tags | No tags attached. | ||||
Attached Files | 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'); | ||||
|
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. |
|
Committed as r2490. |
Date Modified | Username | Field | Change |
---|---|---|---|
2008-07-03 04:37 | Gnomi | New Issue | |
2008-07-03 04:37 | Gnomi | Status | new => assigned |
2008-07-03 04:37 | Gnomi | Assigned To | => Gnomi |
2008-12-26 19:42 | Gnomi | File Added: restore_value.diff | |
2008-12-26 19:44 | Gnomi | Note Added: 0000832 | |
2008-12-26 19:55 | Gnomi | File Deleted: restore_value.diff | |
2008-12-26 19:55 | Gnomi | File Added: restore_value.diff | |
2009-01-14 08:20 | Gnomi | Note Added: 0000890 | |
2009-01-14 08:20 | Gnomi | Status | assigned => resolved |
2009-01-14 08:20 | Gnomi | Fixed in Version | => 3.3.719 |
2009-01-14 08:20 | Gnomi | Resolution | open => fixed |
2010-11-16 09:42 | Gnomi | Source_changeset_attached | => ldmud.git master 4101ca03 |
2018-01-29 18:59 | Gnomi | Source_changeset_attached | => ldmud.git master 4101ca03 |
2018-01-29 21:57 | Gnomi | Source_changeset_attached | => ldmud.git master 4101ca03 |