View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0000553 | LDMud 3.2 | Implementation | public | 2008-07-10 07:31 | 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.2.15 | ||||
Fixed in Version | 3.2.16 | ||||
Summary | 0000553: Backports of 3.3 fixes for 3.2.16 | ||||
Description | This is a bug for collecting suggestions and patches of 3.3 fixes that should be backported to 3.2.16. | ||||
Tags | No tags attached. | ||||
Attached Files | bug537.diff (6,325 bytes)
Index: 3.2/src/prolang.y =================================================================== --- 3.2/src/prolang.y (Revision 2379) +++ 3.2/src/prolang.y (Arbeitskopie) @@ -435,6 +435,10 @@ { int first_local; /* Number of first local defined in this scope */ int num_locals; /* Number of locals defined in this scope */ + int num_cleared; + /* Number of locals that have been cleared by earlier CLEAR_LOCALS */ + Bool clobbered; + /* Local variables beyond num_locals may be clobbered */ mp_uint addr; /* Address of CLEAR_LOCALS instruction, needed for backpatching */ }; @@ -1842,6 +1846,8 @@ { block_scope[depth-1].num_locals = 0; block_scope[depth-1].first_local = current_number_of_locals; + block_scope[depth-1].num_cleared = 0; + block_scope[depth-1].clobbered = MY_FALSE; block_scope[depth-1].addr = 0; } /* init_scope() */ @@ -1865,10 +1871,13 @@ /*-------------------------------------------------------------------------*/ static void -leave_block_scope (void) +leave_block_scope (Bool dontclobber) /* Leave the current scope (if use_local_scopes requires it), freeing * all local names defined in that scope. + * + * <dontclobber> should be MY_TRUE if the stack of the to-be-left scope + * is independent of the outer scope (i.e. the scope of closures). */ { @@ -1876,6 +1885,13 @@ { free_local_names(block_depth); block_depth--; + if (block_depth && !dontclobber + && (block_scope[block_depth].num_locals + || block_scope[block_depth].clobbered)) + { + /* the block we just left may have clobbered local variables */ + block_scope[block_depth-1].clobbered = MY_TRUE; + } } } /* leave_block_scope() */ @@ -4182,17 +4198,17 @@ { block_scope_t *scope = block_scope + block_depth - 1; - if (use_local_scopes && scope->num_locals) + if (use_local_scopes && scope->num_locals > scope->num_cleared) { mem_block[A_PROGRAM].block[scope->addr+2] - = (char)scope->num_locals; + = (char)(scope->num_locals - scope->num_cleared); } } } '}' - { leave_block_scope(); } + { leave_block_scope(MY_FALSE); } ; /* block */ @@ -4326,14 +4342,23 @@ q = add_local_name($2, current_type | $1, block_depth); - if (use_local_scopes && scope->num_locals == 1) + if (use_local_scopes && scope->clobbered) { + /* Finish the previous CLEAR_LOCALS, if any */ + if (scope->num_locals - 1 > scope->num_cleared) + mem_block[A_PROGRAM].block[scope->addr+2] + = (char)(scope->num_locals - 1 - scope->num_cleared); + scope->clobbered = MY_FALSE; + scope->num_cleared = scope->num_locals - 1; + } + if (use_local_scopes && scope->num_locals == scope->num_cleared + 1) + { /* First definition of a local, so insert the * clear_locals bytecode and remember its position */ scope->addr = mem_block[A_PROGRAM].current_size; ins_f_code(F_CLEAR_LOCALS); - ins_byte(scope->first_local); + ins_byte(scope->first_local + scope->num_cleared); ins_byte(0); } @@ -4354,14 +4379,23 @@ q = redeclare_local($2, current_type | $1, block_depth); - if (use_local_scopes && scope->num_locals == 1) + if (use_local_scopes && scope->clobbered) { + /* Finish the previous CLEAR_LOCALS, if any */ + if (scope->num_locals - 1 > scope->num_cleared) + mem_block[A_PROGRAM].block[scope->addr+2] + = (char)(scope->num_locals - 1 - scope->num_cleared); + scope->clobbered = MY_FALSE; + scope->num_cleared = scope->num_locals - 1; + } + if (use_local_scopes && scope->num_locals == scope->num_cleared + 1) + { /* First definition of a local, so insert the * clear_locals bytecode and remember its position */ scope->addr = mem_block[A_PROGRAM].current_size; ins_f_code(F_CLEAR_LOCALS); - ins_byte(scope->first_local); + ins_byte(scope->first_local + scope->num_cleared); ins_byte(0); } @@ -4903,10 +4937,10 @@ { block_scope_t *scope = block_scope + block_depth - 1; - if (use_local_scopes && scope->num_locals) + if (use_local_scopes && scope->num_locals > scope->num_cleared) { mem_block[A_PROGRAM].block[scope->addr+2] - = (char)scope->num_locals; + = (char)(scope->num_locals - scope->num_cleared); } } } @@ -4995,7 +5029,7 @@ current_break_address = $<numbers>3[1]; /* and leave the for scope */ - leave_block_scope(); + leave_block_scope(MY_FALSE); } ; /* for */ @@ -5198,10 +5232,10 @@ { block_scope_t *scope = block_scope + block_depth - 1; - if (use_local_scopes && scope->num_locals) + if (use_local_scopes && scope->num_locals > scope->num_cleared) { mem_block[A_PROGRAM].block[scope->addr+2] - = (char)scope->num_locals; + = (char)(scope->num_locals - scope->num_cleared); } } @@ -5298,7 +5332,7 @@ current_break_address = $<numbers>3[1]; /* and leave the scope */ - leave_block_scope(); + leave_block_scope(MY_FALSE); } ; /* foreach */ @@ -9483,7 +9517,7 @@ ); /* Restore the old locals information */ - leave_block_scope(); + leave_block_scope(MY_TRUE); use_local_scopes = pragma_use_local_scopes; all_locals = save_all_locals; current_number_of_locals = save_current_number_of_locals; restore_value_alloca.diff (2,721 bytes)
Index: 3.2/src/object.c =================================================================== --- 3.2/src/object.c (Revision 2389) +++ 3.2/src/object.c (Arbeitskopie) @@ -5180,9 +5180,12 @@ { int restored_version; /* Formatversion of the saved data */ + Bool allocated_buff; /* buff is xallocated. */ char *buff; /* The string to parse */ char *p; +#define FREE_BUFF() MACRO( if (allocated_buff) xfree(buff); ) + if (sp->type != T_STRING) { bad_xefun_arg(1, sp); @@ -5197,7 +5200,7 @@ size_t len; len = strlen(sp->u.string); - buff = alloca(len+1); + buff = xalloc(len+1); if (!buff) { errorf("(restore) Out of stack (%lu bytes).\n" @@ -5207,9 +5210,13 @@ } memcpy(buff, sp->u.string, len); buff[len] = '\0'; + allocated_buff = MY_TRUE; } else + { buff = sp->u.string; + allocated_buff = MY_FALSE; + } restored_version = -1; restored_host = -1; @@ -5225,11 +5232,14 @@ p = strchr(buff, '\n'); if (!p) { + FREE_BUFF(); errorf("No data given.\n"); return sp-1; } - buff = p+1; + p++; } + else + p = buff; /* parse from beginning of buffer */ /* Initialise the shared value table */ @@ -5244,6 +5254,7 @@ shared_restored_values = xalloc(sizeof(svalue_t)*max_shared_restored); if (!shared_restored_values) { + FREE_BUFF(); errorf("(restore) Out of memory (%lu bytes) for shared values.\n" , max_shared_restored * sizeof(svalue_t)); return sp; /* flow control hint */ @@ -5256,7 +5267,6 @@ /* Now parse the value in buff[] */ - p = buff; if ( (restored_version < 0 && p[0] == '\"') ? !old_restore_string(sp, p) : !restore_svalue(sp, &p, '\n') @@ -5265,6 +5275,7 @@ /* Whoops, illegal format */ free_shared_restored_values(); + FREE_BUFF(); errorf("Illegal format when restoring a value.\n"); /* NOTREACHED */ return sp; /* flow control hint */ @@ -5274,6 +5285,7 @@ if (*p != '\0') { + FREE_BUFF(); errorf("Illegal format when restoring a value: extraneous characters " "at the end.\n"); /* NOTREACHED */ @@ -5282,11 +5294,15 @@ /* Restore complete - now clean up and return the result */ + FREE_BUFF(); + inter_sp = --sp; free_string_svalue(sp); *sp = sp[1]; return sp; + +#undef FREE_BUFF } /* f_restore_value() */ /***************************************************************************/ addmessage_buff.diff (1,459 bytes)
Index: 3.2/src/comm.c =================================================================== --- 3.2/src/comm.c (Revision 2426) +++ 3.2/src/comm.c (Arbeitskopie) @@ -1870,15 +1870,20 @@ } else { - buff[(sizeof buff) - 1] = '\0'; /* Overrun sentinel */ - vsprintf(buff+1,fmt,va); + size_t len; + len = vsnprintf(buff+1, sizeof(buff)-1, fmt,va); va_end(va); - if (buff[(sizeof buff) - 1]) + /* old sprintf() implementations returned -1 if the output was + * truncated. Since size_t is an unsigned type, the check for + * len == -1 is implicitly included by >= sizeof(...)-1, because + * -1 will be wrapped to SIZE_T_MAX which is the maximum sizeof() + * can return and can never be valid as return value here. */ + if (len >= sizeof(buff)-1) { - debug_message("%s Message too long: '%.200s...'\n" - , time_stamp(), buff); - comm_fatal(ip, "Mssage too long!\n"); - return; + char err[] = "\n*** Message truncated ***\n"; + debug_message("%s Message too long (Length: %zu): '%.200s...'\n" + , time_stamp(), len, buff); + (void)memcpy(buff+(sizeof(buff)-sizeof(err)), err, sizeof(err)); } source = buff+1; } | ||||
related to | 0000537 | resolved | Gnomi | LDMud 3.3 | Missing initialization of local variables in some cases |
related to | 0000532 | resolved | zesstra | LDMud 3.3 | restore_value() segfaults on large inputs on 64-bit Debian; alloca() related |
related to | 0000577 | resolved | zesstra | LDMud 3.3 | Potential crashes in send_erq() and send_udp() due to stack overflow |
related to | 0000578 | resolved | zesstra | LDMud 3.3 | Potential crashes in regexplode(), process_string(), present_clone() |
related to | 0000582 | resolved | zesstra | LDMud 3.3 | Potential crash in db_conv_string() due to stack overflow |
parent of | 0000539 | resolved | Gnomi | LDMud 3.2 | Missing initialization of local variables in some cases |
|
I uploaded a straightforward backport for 0000537 (missing F_CLEAR_LOCALs) and I also intend to port 0000532 (alloca in restore_value). |
|
I added a patch for 0000532. I did it similar to restore_object without an error handler. It's not foolproof (see my comment 666 in 0000532), but then again this is oldstable. Should I add error handlers to restore_object and restore_value, nevertheless? |
|
I don't think, that will be necessary to invest more work than strictly necessary. An error should be rare and it will be caught eventually by the garbage collector. That should be acceptable for oldstable. |
|
I'd like to backport the add_message buffer overflow (r2422), too. (Patch attached.) |
|
Bug 0000575 is not applicable as 3.2 doesn't have a filter(string). Backport of 0000577 was committed as r2689. |
|
All remaining backports committed in r2693. |
|
3.2.16 was released a few months ago. |
Date Modified | Username | Field | Change |
---|---|---|---|
2008-07-10 07:31 | Gnomi | New Issue | |
2008-07-10 07:31 | Gnomi | Status | new => assigned |
2008-07-10 07:31 | Gnomi | Assigned To | => Gnomi |
2008-07-10 07:33 | Gnomi | File Added: bug537.diff | |
2008-07-10 07:34 | Gnomi | Note Added: 0000717 | |
2008-07-10 07:36 | Gnomi | Relationship added | related to 0000537 |
2008-07-10 07:36 | Gnomi | Relationship added | related to 0000532 |
2008-07-16 14:26 | Gnomi | File Added: restore_value_alloca.diff | |
2008-07-16 14:30 | Gnomi | Note Added: 0000731 | |
2008-07-16 14:48 | zesstra | Note Added: 0000732 | |
2008-07-16 16:20 | zesstra | Relationship added | parent of 0000539 |
2008-09-22 01:12 | Gnomi | File Added: addmessage_buff.diff | |
2008-09-22 01:13 | Gnomi | Note Added: 0000790 | |
2008-11-17 15:29 | Gnomi | Relationship added | related to 0000575 |
2008-11-17 15:35 | Gnomi | Relationship added | related to 0000577 |
2008-11-17 15:38 | Gnomi | Relationship added | related to 0000578 |
2008-11-17 16:06 | Gnomi | Relationship added | related to 0000582 |
2009-03-12 03:57 | zesstra | Relationship added | related to 0000611 |
2009-03-12 17:08 | zesstra | Relationship deleted | related to 0000611 |
2009-06-23 12:04 | Gnomi | Relationship deleted | related to 0000575 |
2009-06-23 12:05 | Gnomi | Note Added: 0001230 | |
2009-06-23 15:23 | Gnomi | Note Added: 0001231 | |
2009-11-16 04:48 | Gnomi | Note Added: 0001625 | |
2009-11-16 04:48 | Gnomi | Status | assigned => resolved |
2009-11-16 04:48 | Gnomi | Fixed in Version | => 3.2.16 |
2009-11-16 04:48 | Gnomi | Resolution | open => fixed |
2010-11-16 09:42 | Gnomi | Source_changeset_attached | => ldmud.git master-3.2 f31bbfdb |
2010-11-16 09:42 | Gnomi | Source_changeset_attached | => ldmud.git master-3.2 110b92dc |
2018-01-29 18:59 | Gnomi | Source_changeset_attached | => ldmud.git master-3.2 f31bbfdb |
2018-01-29 18:59 | Gnomi | Source_changeset_attached | => ldmud.git master-3.2 110b92dc |
2018-01-29 21:57 | Gnomi | Source_changeset_attached | => ldmud.git master-3.2 f31bbfdb |
2018-01-29 21:57 | Gnomi | Source_changeset_attached | => ldmud.git master-3.2 110b92dc |