424405 - XDR should compensate for traps set in the script it is serializing

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Crowder]

This is a flaw revealed by the recent addition of fastload in Cu.import, and an assertion added in January, but the bug is much older.  Patch and testcase in a bit.

Comment 1

[Brian Crowder]

Attachment: Patch a copy of the XDR data for scripts which have traps

Test patch next.

Comment 2

[Brian Crowder]

Attachment: xdr test patch using JSAPI in C++

This is the first JSAPI test in the make check suite.

Comment 3

[Brian Crowder]

This should surely block 1.9, maybe b5?  The bug here will likely cause people using chrome-debuggers to have difficulty restarting their browser.

Comment 4

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 311086 [details] [diff] [review]
Patch a copy of the XDR data for scripts which have traps

>+    dupcode = NULL;
>+    if (xdr->mode == JSXDR_ENCODE) {
>+        JSTrap *trap;
>+        JSRuntime *rt;
>+
>+        rt = cx->runtime;
>+        DBG_LOCK(rt);

>+        for (trap = (JSTrap *)rt->trapList.next;
>+             trap != (JSTrap *)&rt->trapList;
>+             trap = (JSTrap *)trap->links.next) {
>+            if (trap->script == script && code == script->code) {

code is always == script->code during this loop, and you don't need to check anything but trap->script == script AFAICT; FindTrap compares pc against trap->pc, but you don't need or want to do that here.

>+                if (!dupcode) {
>+                    dupcode = malloc(length * sizeof(jsbytecode));
>+                    if (!dupcode)
>+                        goto error;
>+                    memcpy(dupcode, script->code, length * sizeof(jsbytecode));
>+                }

You could make dupcode a local and set code to equal it after it's duped; then you can free below iff code != script->code, right?  If not, you should set code = dupcode here, rather than lower down, since it's at this point that we know we're going to want to serialize dupcode.  (|dupcode| better named |patched|? the point of it is that it's not a duplicate :) )

>+                dupcode[trap->pc - script->code] = trap->op;
>+            }
>+        }
>+        DBG_UNLOCK(rt);
>+        if (dupcode)
>+            code = dupcode;
>+    }
>+
>     oldscript = xdr->script;
>     xdr->script = script;
>-    if (!JS_XDRBytes(xdr, (char *)script->code, length * sizeof(jsbytecode)))
>+    if (!JS_XDRBytes(xdr, (char *) code, length * sizeof(jsbytecode)))
>         goto error;
> 
>+    if (dupcode)
>+        free(dupcode);

Need to free dupcode on error.  (Free code iff code != script->code, if you follow my changes.)

I'll quickly stamp an r+ on a patch that cleans those up.

Comment 5

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 311087 [details] [diff] [review]
xdr test patch using JSAPI in C++

This doesn't belong in xpcom/tests; recommend js/src/tests for JSAPI tests.

>Index: TestJSAPI.cpp
>===================================================================
>RCS file: TestJSAPI.cpp
>diff -N TestJSAPI.cpp
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ TestJSAPI.cpp	22 Mar 2008 00:11:13 -0000
>@@ -0,0 +1,148 @@
>+
>+#include <jsapi.h>
>+#include <jsdbgapi.h>
>+#include <jsprf.h>
>+#include <jsstddef.h>
>+#include <jsxdrapi.h>
>+
>+#include <memory.h>
>+#include <string.h>
>+
>+#define REPORT_ON_FAIL(expr, report) \
>+    if (!expr) { \
>+        printf("%25s : FAILURE <--\n", report); \
>+        return 0; \
>+    }

Isn't 0 a success code?  (Does this |make check| fail without your patch in place?)

Looks from http://developer.mozilla.org/en/docs/Compiled-code_automated_tests#The_structure_of_a_compiled-code_test
that failed tests should return 1 from main.

Do we need a global object for this test?  I expected it to be much smaller -- we don't need all the warning or newline complexity in the error reporter, if we need an error reporter at all. Don't write a whole test harness here, just write the clearest and most direct test of what you need to verify -- and then check that |make check| can tell whether the bug is fixed or not!

Trimmed down example:

>+    JSRuntime *rt = JS_NewRuntime(64L * 1024L * 1024L);
>+    REPORT_ON_FAIL(rt, "creating runtime");
>+
>+    JSContext *cx = JS_NewContext(rt, 8 * 1024);
>+    REPORT_ON_FAIL(cx, "creating context");
>+
>+    const char testscript[] = "function f() { }";
>+    JSScript *script = JS_CompileScript(cx, glob, testscript,
>+                                        JS_ARRAY_LENGTH(testscript) - 1,
>+                                        "test", 1);

Compile against NULL here, instead.

>+    JSXDRState *xdr = JS_XDRNewMem(cx, JSXDR_ENCODE);
>+    REPORT_ON_FAIL(xdr, "creating xdr encoder");
>+    REPORT_ON_FAIL(JS_XDRScript(xdr, &script), "script xdr");
>+
>+    uint32 length;
>+    const char *data =
>+        static_cast<const char *>(JS_XDRMemGetData(xdr, &length));
>+    char *datacp = new char[length];
>+    memcpy(datacp, data, length);

Don't need to allocate or copy, just use beforeData and afterData in memcmp below.

>+    jsbytecode *pc = JS_LineNumberToPC(cx, script, 0);
>+    REPORT_ON_FAIL(pc, "getting program counter");
>+
>+    JS_SetTrap(cx, script, pc, (JSTrapHandler) JS_HandleTrap, 0);
>+
>+    JSXDRState *xdr2 = JS_XDRNewMem(cx, JSXDR_ENCODE);
>+    REPORT_ON_FAIL(JS_XDRScript(xdr2, &script), "script xdr");
>+    data = static_cast<const char *>(JS_XDRMemGetData(xdr2, &length));
>+
>+    printf("after trap set: %s\n",
>+           memcmp(datacp, data, length) == 0 ? "match" : "mismatch");

If they differ, shouldn't the test fail?

>+    return 0;
>+}

I think this should be a test_bug424405.c or test_xdr.c and written in straight C (without the allocations it's just a matter of changing the casts), found in js/src/tests.  We can do a followup after FF3 to let "make -f Makefile.ref check" run these, too!

Thanks a ton for breaking the ice on these JSAPI unit tests!

Comment 6

[Brian Crowder]

Attachment: addressing shaver's review

The cleanups and fixes you suggested plus the xdrapi changes needed to make this compile.

Comment 7

[Brian Crowder]

(In reply to comment #6)
> The cleanups and fixes you suggested plus the xdrapi changes needed to make
> this compile.

dbgapi changes, of course, not xdrapi

Comment 8

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 311151 [details] [diff] [review]
addressing shaver's review

Use JS_malloc instead of malloc, so you get an error report on OOM and not silent failure; sorry for missing that last time!

>-    if (!JS_XDRBytes(xdr, (char *)script->code, length * sizeof(jsbytecode)))
>+    if (!JS_XDRBytes(xdr, (char *) code, length * sizeof(jsbytecode))) {
>+        if (code != script->code)
>+            free(code);
>         goto error;
>+    }
>+
>+    if (code != script->code)
>+        free(code);

Slightly better might be:

ok = JS_XDRBytes(...args);

if (code != script->code)
  JS_free(code);

if (!ok)
  goto error;

or putting the free case in the error handler.  Not a show-stopper; r+ granted with the changes to JS_free/JS_malloc, follow your heart on code duplication of freeing.

Comment 9

[Brian Crowder]

Attachment: the patch to land

Obsoleting the test patch per discussion with shaver.  We need infrastructure to do this right in js/src/tests.  (new bug?)

Comment 10

[Brian Crowder]

If this gets b5 approval, I'd very much appreciate someone else landing it for me today.  I'll be out until later tonight.

Comment 11

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 311154 [details] [diff] [review]
the patch to land

Pow! Right in the b5 approval!

(ab5=shaver)

Comment 12

[Bob Clary [:bc] (inactive)]

(In reply to comment #9)

> Obsoleting the test patch per discussion with shaver.  We need infrastructure
> to do this right in js/src/tests.  (new bug?)

yes please.

Comment 13

[Reed Loden [:reed]]

Checking in js/src/jsdbgapi.c;
/cvsroot/mozilla/js/src/jsdbgapi.c,v  <--  jsdbgapi.c
new revision: 3.137; previous revision: 3.136
done
Checking in js/src/jsdbgapi.h;
/cvsroot/mozilla/js/src/jsdbgapi.h,v  <--  jsdbgapi.h
new revision: 3.45; previous revision: 3.44
done
Checking in js/src/jsscript.c;
/cvsroot/mozilla/js/src/jsscript.c,v  <--  jsscript.c
new revision: 3.175; previous revision: 3.174
done

Comment 14

[Brendan Eich [:brendan]]

Comment on attachment 311154 [details] [diff] [review]
the patch to land

>Index: jsdbgapi.h
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/jsdbgapi.h,v
>retrieving revision 3.44
>diff -u -p -8 -r3.44 jsdbgapi.h
>--- jsdbgapi.h	19 Jan 2008 23:33:08 -0000	3.44
>+++ jsdbgapi.h	22 Mar 2008 16:33:51 -0000
>@@ -38,21 +38,35 @@
>  * ***** END LICENSE BLOCK ***** */
> 
> #ifndef jsdbgapi_h___
> #define jsdbgapi_h___
> /*
>  * JS debugger API.
>  */
> #include "jsapi.h"
>+#include "jsclist.h"
> #include "jsopcode.h"
> #include "jsprvtd.h"
> 
> JS_BEGIN_EXTERN_C
> 
>+typedef struct JSTrap {
>+    JSCList         links;
>+    JSScript        *script;
>+    jsbytecode      *pc;
>+    JSOp            op;
>+    JSTrapHandler   handler;
>+    void            *closure;
>+} JSTrap;
>+
>+#define DBG_LOCK(rt)            JS_ACQUIRE_LOCK((rt)->debuggerLock)
>+#define DBG_UNLOCK(rt)          JS_RELEASE_LOCK((rt)->debuggerLock)
>+#define DBG_LOCK_EVAL(rt,expr)  (DBG_LOCK(rt), (expr), DBG_UNLOCK(rt))

None of this is "API" so it should not be unifdef'ed here in jsdbgapi.h. Really, jsscript.c just wanted a js_UntrapScriptCode or some such.

Please file a followup bug on this.

/be

Comment 15

[Brendan Eich [:brendan]]

Worse, this patch introduced a warning:

made:jsscript.c:420: warning: 'oldscript' may be used uninitialized in this function

which is a real bug -- OOM JS_malloc'ing the copy of script->code to untrap will goto error with oldscript uninitialized. Grumpy about this whole situation, I went and patched. File the followup bug, make it block this resolved bug, and I will put that patch up.

/be

Comment 16

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

When I was reviewing, I thought about asking for JSTrap to be put in jsprvtd.h, and even for the "temporarily unpatch script->code" logic to be in jsdbgapi, but decided against it:

- jsdbgapi.h includes jsprvtd.h, so I wasn't too concerned about polluting it with JSTrap internals
- providing a way to lock across multiple dbgapi operations for consistency seemed useful -- I now realize that other callers would be begging for deadlock, and that of course I should have then asked for a more appropriate name :(
- really, I think that jsscript.c should have the opcode patching in it, and jsdbgapi.c should call into it, rather than prodding in script->code directly, but I didn't want to block this b5 fix on that rearchitecture, especially given the dire state of debugger test coverage.

I should definitely have put that reasoning in the bug for scrutiny, if nothing else, but I had a baby on my lap at the time, and am a bit lame.

I should definitely also have caught the flow-control problem on review. :(

Bug 424614 is filed as requested.  Apologies.

Comment 17

[Brendan Eich [:brendan]]

(In reply to comment #16)
> - jsdbgapi.h includes jsprvtd.h, so I wasn't too concerned about polluting it
> with JSTrap internals

"td" for typedef -- jsprvtd.h (perverted typedefs? ;-) is mostly opaque struct tag typedefs, not nearly as private as private struct members in full -- but it's true that jsdbgapi.h has less honor because of this jsprvtd.h include.

> - really, I think that jsscript.c should have the opcode patching in it, and
> jsdbgapi.c should call into it,

jsdbgapi.c already includes jsscript.h -- small-world network modularity, and in fact js_PatchOpcode (long unused, never exported as FRIEND or PUBLIC, and now removed in the patch for bug 424614 to help offset the code size hit) lived in jsdbgapi.c precisely to help isolate bytecode patching to that file.

> I should definitely have put that reasoning in the bug for scrutiny, if nothing
> else, but I had a baby on my lap at the time, and am a bit lame.

Not to worry! Babies come first, just don't let her type!

/be

Comment 18

[chris hofmann]

check bug 431724 for a possible regression on this check-in

Comment 19

[Brian Crowder]

Clearing my blocking request from ages ago.