View Issue Details

IDProjectCategoryView StatusLast Update
0000575LDMud 3.3Implementationpublic2018-01-29 21:57
Reporterzesstra Assigned Tozesstra  
PriorityhighSeveritycrashReproducibilityalways
Status resolvedResolutionfixed 
Platformx86_64OSMacOS XOS Version10.5.x
Product Version3.3 
Fixed in Version3.3.718 
Summary0000575: filter(<string>,...) may crash with large strings
Descriptionfilter() allocates a temporary buffer on the stack which has the size of the given string. Because stack memory is usually a limited ressource and alloca() has no error handling (does not return NULL if the stack limit would be exceeded) but just moves the stack pointer, after the allocation either non-allocated memory areas or system libs or the heap might be overwritten.
Steps To ReproduceAssuming the standard 8MB stack:
filter("a")*8000000,#'intp);
TagsNo tags attached.
Attached Files
errorhandler.diff (4,692 bytes)   
Index: interpret.h
===================================================================
--- interpret.h	(Revision 2411)
+++ interpret.h	(Arbeitskopie)
@@ -80,6 +80,16 @@
        */
 };
 
+/* a general error handler structure. head is assigned as payload to an 
+ * T_LVALUE svalue of type T_ERROR_HANDLER and pushed onto the value stack.
+ * If the stack is unrolled during runtime errors the error_handler function
+ * is called and frees buff. */
+typedef struct errorhandler_s {
+  svalue_t head;        /* The T_ERROR_HANDLER structure */
+  char     * buff;      /* The allocated buffer to free. */
+} errorhandler_t;
+
+
 /* --- Macros --- */
 
 #define MAX_SHIFT ((sizeof(p_int) << 3) - 1)
@@ -147,6 +157,11 @@
 extern void push_svalue_block(int num, svalue_t *v);
 extern svalue_t *pop_n_elems (int n, svalue_t *sp);
 extern void pop_stack(void);
+extern void push_apply_value(void);
+extern void pop_apply_value (void);
+extern void push_referenced_mapping(mapping_t *m);
+extern void push_error_handler(void (*errorhandler)(svalue_t *), svalue_t *arg);
+extern void *alloc_with_error_handler(size_t size);
 
 extern void init_interpret(void);
 extern const char *typename(int type);
@@ -161,8 +176,7 @@
 extern Bool privilege_violation(string_t *what, svalue_t *arg, svalue_t *sp);
 extern Bool privilege_violation2(string_t *what, svalue_t *arg, svalue_t *arg2, svalue_t *sp);
 extern Bool privilege_violation4(string_t *what, object_t *whom, string_t *how_str, int how_num, svalue_t *sp);
-extern void push_apply_value(void);
-extern void pop_apply_value (void);
+
 extern svalue_t *sapply_int(string_t *fun, object_t *ob, int num_arg, Bool b_ign_prot, Bool b_use_default);
 #define sapply(f,o,n) sapply_int(f,o,n, MY_FALSE, MY_TRUE)
 #define sapply_ign_prot(f,o,n) sapply_int(f,o,n, MY_TRUE, MY_TRUE)
@@ -193,8 +207,6 @@
 extern inherit_t *adjust_variable_offsets(const inherit_t *inheritp, const program_t *prog, const object_t *obj);
 extern void free_interpreter_temporaries(void);
 extern void invalidate_apply_low_cache(void);
-extern void push_referenced_mapping(mapping_t *m);
-extern void push_error_handler(void (*errorhandler)(svalue_t *), svalue_t *arg);
 extern void m_indices_filter (svalue_t *key, svalue_t *data, void *extra);
 extern void m_values_filter (svalue_t *key, svalue_t *data, void *extra);
 extern void m_unmake_filter ( svalue_t *key, svalue_t *data, void *extra);
Index: interpret.c
===================================================================
--- interpret.c	(Revision 2426)
+++ interpret.c	(Arbeitskopie)
@@ -2720,6 +2720,7 @@
     return r;
 } /* inter_add_array() */
 
+
 /*=========================================================================*/
 
 /*                           S T A C K                                     */
@@ -6417,6 +6418,55 @@
 } /* test_efun_args() */
 
 
+/*-------------------------------------------------------------------------*/
+/* general errorhandler */
+static void
+general_error_handler( svalue_t * arg)
+/* The error handler: free the allocated buffer and the errorhandler structure.
+ * Note: it is static, but the compiler will have to emit a function and 
+ * symbol for this because the address of the function is taken and it is 
+ * therefore not suitable to be inlined.
+ */
+{
+  errorhandler_t *handler = (errorhandler_t *)arg;
+  if (handler->buff)
+    xfree(handler->buff);
+  xfree(handler);
+} /* general_error_handler() */
+
+/*-------------------------------------------------------------------------*/
+void *
+alloc_with_error_handler(size_t size)
+/* Allocates <size> bytes from the heap. Additionally an error handler is
+ * pushed onto the value stack so that the requested memory is safely freed,
+ * either by manually freeing the svalue on the stack or during stack 
+ * unwinding during errorf().
+ * inter_sp has to point to the top-of-stack before calling and is updated to
+ * point to the error handler svalue (new top-of-stack)!
+ */
+{
+  void *buffer;
+  errorhandler_t *handler;
+  /* get the memory for the handler first and fail if out-of-memory */
+  handler = xalloc(sizeof(*handler));
+  if (!handler)
+  {
+    return NULL;
+  }
+  /* then get the requested memory - upon error de-allocate the handler */
+  buffer = xalloc(size);
+  if (!buffer)
+  {
+    xfree(handler);
+    return NULL;
+  }
+  handler->buff = buffer;
+  /* now push error handler onto the value stack */
+  push_error_handler(general_error_handler, &(handler->head));
+  return buffer;
+} /* alloc_with_error_handler */
+
+
 /*=========================================================================*/
 /*-------------------------------------------------------------------------*/
 Bool
errorhandler.diff (4,692 bytes)   
filterV2.diff (4,454 bytes)   
Index: strfuns.c
===================================================================
--- strfuns.c	(Revision 2418)
+++ strfuns.c	(Arbeitskopie)
@@ -896,14 +896,6 @@
     str = arg->u.str;
     slen = (mp_int)mstrsize(str);
 
-    flags = alloca((size_t)slen+1);
-    if (!flags)
-    {
-        errorf("Stack overflow in filter()");
-        /* NOTREACHED */
-        return sp;
-    }
-
     /* Every element in flags is associated by index number with an
      * element in the vector to filter. The filter function is evaluated
      * for every string character, and the associated flag is set to 0
@@ -920,11 +912,21 @@
         mapping_t *m;
 
         if (num_arg > 2) {
-            inter_sp = sp;
             errorf("Too many arguments to filter(array)\n");
         }
+        /* Allocate memory for the flag array. Simultaneously an error
+         * handler is pushed onto the stack (after the arguments) for freeing
+         * the buffer in case of runtime errors. */
+        flags = alloc_with_error_handler((size_t)slen + 1);
+        if (!flags)
+        {
+          errorf("Out of memory (%zu bytes) for temporary buffer in filter().\n",
+                 (size_t)slen + 1);
+        }
+        sp = inter_sp;
+
         m = arg[1].u.map;
-
+        
         for (src = get_txt(str), cnt = slen; --cnt >= 0; src++)
         {
             svalue_t key;
@@ -939,9 +941,6 @@
             res++;
         }
 
-        free_svalue(arg+1); /* the mapping */
-        sp = arg;
-
     } else {
 
         /* --- Filter by function call --- */
@@ -951,19 +950,33 @@
         mp_int cnt;
 
         assign_eval_cost();
-        inter_sp = sp;
 
+        /* setup_efun_callback() will adopt and therefore remove the 
+         * arguments from arg+1 on to arg+num_arg from the stack and update 
+         * inter_sp. New top-of-stack will be arg. */
         error_index = setup_efun_callback(&cb, arg+1, num_arg-1);
-
         if (error_index >= 0)
         {
             vefun_bad_arg(error_index+2, arg);
             /* NOTREACHED */
             return arg;
         }
-        inter_sp = sp = arg+1;
+        /* push the callback structure onto the stack. */
+        sp = arg + 1;
         put_callback(sp, &cb);
 
+        /* Allocate memory for the flag array. Simultaneously an error
+         * handler is pushed onto the stack (after the arguments) for freeing
+         * the buffer in case of runtime errors. */
+        inter_sp = sp;
+        flags = alloc_with_error_handler((size_t)slen + 1);
+        if (!flags)
+        {
+            errorf("Out of memory (%zu bytes) for temporary buffer in filter().\n",
+                   (size_t)slen + 1);
+        }
+        sp = inter_sp;
+        
         /* Loop over all elements in p and call the filter.
          * w is the current element filtered.
          */
@@ -994,32 +1007,37 @@
             flags[cnt] = 1;
             res++;
         }
-
-        free_callback(&cb);
     }
 
     /* flags[] holds the filter results, res is the number of
      * elements to keep. Now create the result vector.
      */
     rc = alloc_mstring(res);
-    if (rc)
+    if (!rc)
     {
-        for (src = get_txt(str), dest = get_txt(rc), flags = &flags[slen]
-            ; res > 0 ; src++)
+        errorf("Out of memory (%zu bytes) for result in filter().\n",slen+1);
+    }
+  
+    for (src = get_txt(str), dest = get_txt(rc), flags = &flags[slen]
+       ; res > 0 ; src++)
+    {
+        if (*--flags)
         {
-            if (*--flags)
-            {
-                *dest++ = *src;
-                res--;
-            }
+            *dest++ = *src;
+            res--;
         }
     }
+  
+    /* Cleanup. Arguments for the closure have already been removed. On the
+     * stack are now the string, the mapping or callback structure and the
+     * error handler. (Not using a loop for 2 elements for saving loop and 
+     * function call overhead.) */
+    free_svalue(sp--);  /* errorhandler, buffer and flags are freed by this. */
+    free_svalue(sp--);  /* mapping or callback structure. */
+    free_mstring(str);  /* string, at arg == sp */
+    sp->u.str = rc;     /* put result here */
 
-    /* Cleanup (everything but the string has been removed already) */
-    free_mstring(str);
-    arg->u.str = rc;
-
-    return arg;
+    return sp;
 } /* x_filter_string() */
 
 /*-------------------------------------------------------------------------*/
filterV2.diff (4,454 bytes)   

Relationships

child of 0000545 new Usages of alloca() have to be checked for possible stack overflow 

Activities

zesstra

2008-09-23 04:49

administrator   ~0000791

First draft of a patch is attached. It introduces an error handler and cleanup structure and unfortunately involves a transfer of the error handler on the stack, because the arguments are partially removed from the stack in the meantime. If someone devises a simpler strategy... ;-)
Ah, and as a note: the patch uses free_svalue to free the error handler and in that process the cleanup is done. It can be done manually without free_svalue (see patch), but I think, it is cleaner in this way.

zesstra

2008-09-24 13:08

administrator   ~0000792

Gnomi suggested to introduce a general error handler and a function for allocating memory from the heap and pushing an error handler for freeing that memory at the same time.
One suggestion for this is attached as errorhandler.diff.
filterV2.diff is an updated patch which makes use of this.
Additionally the patch fixes a bug in filter() if there is is not enough memory for the result string. In this case filter() used to push 0 as string onto the value stack. Now the proper runtime error is raised.

zesstra

2008-09-26 16:06

administrator   ~0000794

Should be fixed in r2434.

Issue History

Date Modified Username Field Change
2008-09-23 01:27 zesstra New Issue
2008-09-23 01:27 zesstra Status new => assigned
2008-09-23 01:27 zesstra Assigned To => zesstra
2008-09-23 01:29 zesstra File Added: filter.diff
2008-09-23 04:49 zesstra Note Added: 0000791
2008-09-23 04:54 zesstra Relationship added child of 0000545
2008-09-24 13:03 zesstra File Added: errorhandler.diff
2008-09-24 13:04 zesstra File Added: filterV2.diff
2008-09-24 13:04 zesstra File Deleted: filter.diff
2008-09-24 13:08 zesstra Note Added: 0000792
2008-09-26 16:06 zesstra Status assigned => resolved
2008-09-26 16:06 zesstra Fixed in Version => 3.3.718
2008-09-26 16:06 zesstra Resolution open => fixed
2008-09-26 16:06 zesstra Note Added: 0000794
2008-11-17 15:29 Gnomi Relationship added related to 0000553
2009-06-23 12:04 Gnomi Relationship deleted related to 0000553
2010-11-16 09:42 zesstra Source_changeset_attached => ldmud.git master c53ee840
2018-01-29 18:59 zesstra Source_changeset_attached => ldmud.git master c53ee840
2018-01-29 21:57 zesstra Source_changeset_attached => ldmud.git master c53ee840