Author Topic: fluffos-2.23 patch for always-reproducable segfault when parser tracing.  (Read 1724 times)

Offline harik

  • Acquaintance
  • *
  • Posts: 1
    • View Profile
I hit this when using parser debug to figure out why my commands weren't working, then found everything was exploding/failing randomly.  Quick code walk on how it happens:

Code: [Select]
ret = parallel_process_answer(state, apply(func, ob, args, ORIGIN_DRIVER), which);

Code: [Select]
svalue_t *apply (const char * fun, object_t * ob, int num_arg,
                   int where)
{
...
return &apply_ret_value;
apply_ret_value = *sp--;
}

Code: [Select]
static int parallel_process_answer (parse_state_t * state, svalue_t * sv,
                             int which) {
    if (!sv) return 0;
    if (sv->type == T_NUMBER) {
        DEBUG_P(("Return value was: %li", sv->u.number));
        if (sv->u.number)
            return 1;
       
        if (state->num_errors == 0)
            make_error_message(which, &parallel_error_info);
        return -1;
    }
    if (sv->type != T_STRING) {
        DEBUG_P(("Return value was not a string or number.", sv->u.number));
        return 0;
    }
    DEBUG_P(("Returned string was: %s", sv->u.string));

    free_parser_error(&parallel_error_info);
    if (sv->u.string[0] == '#') {

Code: [Select]
static void debug_parse(char *fmt, ...) {
...
tell_object(command_giver, buf, strlen(buf));

Anyone spot the problem from the code I highlighted?

apply() returns a pointer to the lpc stack, which parallel_process_answer tests the type of. If the result is either a string or a number, it calls debug_parse which tries to call catch_tell() on the caller.  If that function exists, it jumps back into lpc - trashing the stack.  When catch_tell returns, the top of the stack is now random, probably a T_NUMBER value 0 (catch_tell returns void, so it's really whatever ends up there).  One of two things can go wrong at this point: If it was a T_NUMBER before the debug, the number is now probably 0, which means your can_whatever() return went from a success to a fail and your verb mysteriously doesn't work - even though the debug trace shows it returning 1!  If it returns a string errormessage though, now you're in trouble: it looks at the sv->u.string after tell_object() trashes the stack, so it tries to read a null pointer on the line "if (sv->u.string[0] == '#') {" and bye-bye driver.

This only happens if parser_debugging is on and the command-issuer has a catch_tell on them - which means that if someone creates a catch_tell shadowing object and tries to use it, the driver will crash the first time any verb would return an error message.  Parse debugging is on by default in the darksouls distribution, so this is probably on a lot of systems who may have never noticed it.  While shadows are admin-create only, if you charm a mob with a catch_tell() routine and order it to do something that'd generate an error, it may crash.  Not sure if it's smart enough to send the traces back to the originator or not.

Patch:
Code: [Select]
Author: Harik <mud-dev@harik.welp.gs>
Date:   Tue Jun 5 06:42:03 2012 -0400

    driver fix for parse trace crashes.

diff --git a/fluffos-2.23-ds01/packages/parser.c b/fluffos-2.23-ds01/packages/parser.c
index 0534f90..35f5107 100644
--- a/fluffos-2.23-ds01/packages/parser.c
+++ b/fluffos-2.23-ds01/packages/parser.c
@@ -1620,13 +1622,17 @@ static void make_error_message (int which, parser_error_t * err) {
  */
 static int process_answer (parse_state_t * state, svalue_t * sv,
                              int which) {
-    if (!sv) return 0;
+    if (!sv) {
+ DEBUG_P(("Process answer: sv == null"));
+ return 0;
+ }
     if (sv->type == T_NUMBER) {
-        DEBUG_P(("Return value was: %i", sv->u.number));
-        if (sv->u.number)
+ int ret = sv->u.number;
+        DEBUG_P(("p_a Return value was: %i", sv->u.number));
+        if (ret)
             return 1;
         if (state->num_errors == best_num_errors) {
-            DEBUG_P(("Have a better match; aborting ..."));
+            DEBUG_P(("p_a Have a better match; aborting ..."));
             return -3;
         }
         if (state->num_errors++ == 0)
@@ -1635,12 +1641,12 @@ static int process_answer (parse_state_t * state, svalue_t * sv,
         return -2;
     }
     if (sv->type != T_STRING) {
-        DEBUG_P(("Return value was not a string or number."));
+        DEBUG_P(("p_a Return value was not a string or number."));
         return 0;
     }
-    DEBUG_P(("Returned string was: %s", sv->u.string));
+    DEBUG_P(("p_a Returned string was: %s", sv->u.string));
     if (state->num_errors == best_num_errors) {
-        DEBUG_P(("Have a better match; aborting ..."));
+        DEBUG_P(("p_a Have a better match; aborting ..."));
         return -3;
     }
     if (state->num_errors++ == 0) {
@@ -1657,10 +1663,14 @@ static int process_answer (parse_state_t * state, svalue_t * sv,
  */
 static int parallel_process_answer (parse_state_t * state, svalue_t * sv,
                              int which) {
-    if (!sv) return 0;
+    if (!sv) {
+ DEBUG_P(("p_p_a sv was null"));
+ return 0;
+ }
     if (sv->type == T_NUMBER) {
+ int ret=sv->u.number;
         DEBUG_P(("Return value was: %li", sv->u.number));
-        if (sv->u.number)
+        if (ret)
             return 1;
         
         if (state->num_errors == 0)
@@ -1671,16 +1681,17 @@ static int parallel_process_answer (parse_state_t * state, svalue_t * sv,
         DEBUG_P(("Return value was not a string or number.", sv->u.number));
         return 0;
     }
-    DEBUG_P(("Returned string was: %s", sv->u.string));
 
     free_parser_error(&parallel_error_info);
     if (sv->u.string[0] == '#') {
         parallel_error_info.error_type = ERR_ALLOCATED;
         parallel_error_info.err.str = string_copy(sv->u.string + 1, "process_answer");
+ DEBUG_P(("p_p_a Returned string was: %s", sv->u.string));
         return -1;
     } else {
         parallel_error_info.error_type = ERR_ALLOCATED;
         parallel_error_info.err.str = string_copy(sv->u.string, "process_answer");
+ DEBUG_P(("p_p_a Returned string was: %s", sv->u.string));
         return 1;
     }
 }

What I did was move the DEBUG_P calls after it's done with the sv, so that the lpc stack-trashing doesn't matter.
Technically apply() should return a copy but since it's used everywhere that'd be a nightmare of memory leaks.

I also annotated the function in the debug calls since multiple functions had the exact same output, a lot easier to follow the path of the parser when mucking with new actions.