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 |