354135 - jsxml.c does not check for too deep recursion

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

jsxml.c in functions like DeepCopy, DescendantsHelper uses C recursion to walk through XML tree but lacks checks for too-shallow C stack.

Comment 1

[Igor Bukanov]

Attachment: v1

The patch adds missing checks for low C stack.

Comment 2

[Brendan Eich [:brendan]]

Oops, cvs error got captured as patch (stderr redirected too?).

/be

Comment 3

[Igor Bukanov]

Attachment: v1 for real

Comment 4

[Igor Bukanov]

(In reply to comment #2)
>  (stderr redirected too?).

I use Emacs' eshell (pseudo shell that use Lisp instead of starting a real shell process). It does not implement a separated stderr redirection and sends both stderr and stdout to the same file :( 

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 299887 [details] [diff] [review]
v1 for real

Worth moving CHECK_RECURSION macro from jsparse.c to jscntxt.h and using it here (and elsewhere too)?

/be

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 299887 [details] [diff] [review]
v1 for real

Worth moving CHECK_RECURSION macro from jsparse.c to jscntxt.h and using it here (and elsewhere too)?

/be

Comment 7

[Igor Bukanov]

Attachment: v2

The patch adds JS_CHECK_RECURSION that allows to check for recursion in one line. The only place that still uses JS_CHECK_STACK_SIZE/js_ReportOverRecursed pair is js_Interpret:

    if (!JS_CHECK_STACK_SIZE(cx, stackDummy)) {
        js_ReportOverRecursed(cx);
        ok = JS_FALSE;
        goto out2;
    }

Using JS_CHECK_RECURSION here would require:

JS_CHECK_RECURSION(cx, ok = JS_FALSE; goto out2);

but I am not sure that this is a valid C even if GCC swallows it.

Comment 8

[Brendan Eich [:brendan]]

That's valid C -- macro actual params are comma-separated, can contain ; -- so you could use CHECK_RECURSION in js_Interpret. I'm in favor if you can stand it. Otherwise, as in the patch, the actual param always contains return followed by either NULL or JS_FALSE, which suggests make the macro more "syntactic" by taking just the JS_FALSE or NULL value, and putting the return in the macro body. But again, I'm in favor of maximal macro factoring madness ;-).

/be

Comment 9

[Igor Bukanov]

Attachment: v3

The new version of the patch uses JS_CHECK_RECURSION in jsinterp.c:

+    JS_CHECK_RECURSION(cx, ok = JS_FALSE; goto out2);

I also moved the macro definition in jscntxt.h right after the definition of JS_STACK_GROWTH_DIRECTION. Here is a full delta:

--- /home/igor/m/jsshell/mozilla/quilt.z15927/js/src/jscntxt.h	2008-01-29 10:24:59.000000000 -0800
+++ js/src/jscntxt.h	2008-01-29 10:18:11.000000000 -0800
@@ -940,29 +940,16 @@ extern void
 js_ReportOutOfMemory(JSContext *cx);
 
 /*
  * Report that cx->scriptStackQuota is exhausted.
  */
 extern void
 js_ReportOutOfScriptQuota(JSContext *cx);
 
-extern void
-js_ReportOverRecursed(JSContext *cx);
-
-#define JS_CHECK_RECURSION(cx, onerror)                                       \
-    JS_BEGIN_MACRO                                                            \
-        int stackDummy_;                                                      \
-                                                                              \
-        if (!JS_CHECK_STACK_SIZE(cx, stackDummy_)) {                          \
-            js_ReportOverRecursed(cx);                                        \
-            onerror;                                                          \
-        }                                                                     \
-    JS_END_MACRO
-
 /*
  * Report an exception using a previously composed JSErrorReport.
  * XXXbe remove from "friend" API
  */
 extern JS_FRIEND_API(void)
 js_ReportErrorAgain(JSContext *cx, const char *message, JSErrorReport *report);
 
 extern void
@@ -1007,16 +994,29 @@ extern JSErrorFormatString js_ErrorForma
  * reasons pre-dating autoconf usage).
  */
 #if JS_STACK_GROWTH_DIRECTION > 0
 # define JS_CHECK_STACK_SIZE(cx, lval)  ((jsuword)&(lval) < (cx)->stackLimit)
 #else
 # define JS_CHECK_STACK_SIZE(cx, lval)  ((jsuword)&(lval) > (cx)->stackLimit)
 #endif
 
+#define JS_CHECK_RECURSION(cx, onerror)                                       \
+    JS_BEGIN_MACRO                                                            \
+        int stackDummy_;                                                      \
+                                                                              \
+        if (!JS_CHECK_STACK_SIZE(cx, stackDummy_)) {                          \
+            js_ReportOverRecursed(cx);                                        \
+            onerror;                                                          \
+        }                                                                     \
+    JS_END_MACRO
+
+extern void
+js_ReportOverRecursed(JSContext *cx);
+
 /*
  * Update the operation counter according to the given weight and call the
  * operation callback when we reach the operation limit. To make this
  * frequently executed macro faster we decrease the counter from
  * JSContext.operationLimit and compare against zero to check the limit.
  *
  * This macro can run the full GC. Return true if it is OK to continue and
  * false otherwise.
--- /home/igor/m/jsshell/mozilla/quilt.z15927/js/src/jsinterp.c	2008-01-29 10:24:59.000000000 -0800
+++ js/src/jsinterp.c	2008-01-29 10:19:16.000000000 -0800
@@ -1854,17 +1854,16 @@ js_Interpret(JSContext *cx, jsbytecode *
 #if JS_HAS_EXPORT_IMPORT
     JSIdArray *ida;
 #endif
     jsint low, high, off, npairs;
     JSBool match;
 #if JS_HAS_GETTER_SETTER
     JSPropertyOp getter, setter;
 #endif
-    int stackDummy;
 
 #ifdef __GNUC__
 # define JS_EXTENSION __extension__
 # define JS_EXTENSION_(s) __extension__ ({ s; })
 #else
 # define JS_EXTENSION
 # define JS_EXTENSION_(s) s
 #endif
@@ -1995,21 +1994,17 @@ js_Interpret(JSContext *cx, jsbytecode *
         interruptHandler = (cx)->debugHooks->interruptHandler;                \
         LOAD_JUMP_TABLE();                                                    \
     JS_END_MACRO
 
     LOAD_INTERRUPT_HANDLER(cx);
 
     /* Check for too much js_Interpret nesting, or too deep a C stack. */
     ++cx->interpLevel;
-    if (!JS_CHECK_STACK_SIZE(cx, stackDummy)) {
-        js_ReportOverRecursed(cx);
-        ok = JS_FALSE;
-        goto out2;
-    }
+    JS_CHECK_RECURSION(cx, ok = JS_FALSE; goto out2);
 
     /*
      * Allocate operand and pc stack slots for the script's worst-case depth,
      * unless we're called to interpret a part of an already active script, a
      * filtering predicate expression for example.
      */
     depth = (jsint) script->depth;
     if (JS_LIKELY(!fp->spbase)) {

Comment 10

[Igor Bukanov]

(In reply to comment #8)
> But again, I'm in favor of maximal macro factoring madness ;-).

I prefer not hide the return statements behind the macro. It makes way too easy to miss an early return after changing the code to require goto out. Plus  

  JS_CHECK_RECURSION(cx, ok = JS_FALSE; goto out2);

does nor look mad for my eyes.

Comment 11

[Brendan Eich [:brendan]]

(In reply to comment #10)
> I prefer not hide the return statements behind the macro. It makes way too easy
> to miss an early return after changing the code to require goto out. Plus  
> 
>   JS_CHECK_RECURSION(cx, ok = JS_FALSE; goto out2);
> 
> does nor look mad for my eyes.

Agreed on both points -- macro-hidden returns are something I use sparingly, only in file-local or function-local settings either without unwind-protect obligations, or with a goto out pattern from the get-go.

/be

Comment 12

[Igor Bukanov]

Comment on attachment 300064 [details] [diff] [review]
v3

The patch fixes stack overflow segfaults through specially crafted XML.

Comment 13

[Igor Bukanov]

I checked in the patch from comment 9 to the trunk:

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1201845511&maxdate=1201845678&who=igor%25mir2.org