1304523 - Debugger.prototype.findScripts doesn't thoroughly validate query 'source' properties

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

Debugger.prototype.findScripts should reject query 'source' properties whose values are Debugger.Source prototypes, or Debugger.Source instances that belong to a Debugger instance other than the one we're invoking findScripts on.

Comment 1

[Jim Blandy :jimb]

Attachment: Make Debugger.prototype.findScripts validate query 'source' property.

Comment 2

[Jim Blandy :jimb]

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5a6660fa093

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8793513 [details] [diff] [review]
Make Debugger.prototype.findScripts validate query 'source' property.

Review of attachment 8793513 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit-test/tests/debug/Debugger-findScripts-24.js
@@ +1,1 @@
> +// findScripts should reject Debugger.Source objects from other Debuggers.

Nice test

::: js/src/vm/Debugger.cpp
@@ +4111,5 @@
>              }
>  
> +            Value owner = (debuggerSource.toObject()
> +                           .as<NativeObject>()
> +                           .getReservedSlot(JSSLOT_DEBUGSOURCE_OWNER));

(Why is this wrapped in parens?)

@@ +4112,5 @@
>  
> +            Value owner = (debuggerSource.toObject()
> +                           .as<NativeObject>()
> +                           .getReservedSlot(JSSLOT_DEBUGSOURCE_OWNER));
> +            if (!owner.isObject()) {

I know we use the only-non-prototype-debugger-objects-have-the-slots-defined invariant all over the place but even so, I had to look at the error message before I realized what was going on here.

I think this could use a comment, or if we want to go the extra mile, we could throw this in Debugger-inl.h:

    inline bool isDebuggerSourcePrototype(const NativeObject& obj) {
        return !obj.getReservedSlot(JSSLOT_DEBUGSOURCE_OWNER).isObject();
    }

@@ +4120,5 @@
> +            }
> +
> +            if (&owner.toObject() != debugger->object) {
> +                JS_ReportErrorNumber(cx, GetErrorMessage, nullptr,
> +                                     JSMSG_DEBUG_WRONG_OWNER, "Debugger.Source");

This is a bit of an aside/curiosity, because I agree that we should throw an error on principle if people are mixing up their Debugger objects, but do we actually rely on this property holding for the script query?

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Also, pretty sure I originally added the source query property, so sorry to everyone involved :-/

Comment 5

[Jim Blandy :jimb]

Thanks for the review!

(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #3)
> @@ +4111,5 @@
> >              }
> >  
> > +            Value owner = (debuggerSource.toObject()
> > +                           .as<NativeObject>()
> > +                           .getReservedSlot(JSSLOT_DEBUGSOURCE_OWNER));
> 
> (Why is this wrapped in parens?)

To make Emacs re-indent it properly by default. I took out the parens.


> @@ +4112,5 @@
> >  
> > +            Value owner = (debuggerSource.toObject()
> > +                           .as<NativeObject>()
> > +                           .getReservedSlot(JSSLOT_DEBUGSOURCE_OWNER));
> > +            if (!owner.isObject()) {
> 
> I know we use the only-non-prototype-debugger-objects-have-the-slots-defined
> invariant all over the place but even so, I had to look at the error message
> before I realized what was going on here.

I added comments to the checks.


> @@ +4120,5 @@
> > +            }
> > +
> > +            if (&owner.toObject() != debugger->object) {
> > +                JS_ReportErrorNumber(cx, GetErrorMessage, nullptr,
> > +                                     JSMSG_DEBUG_WRONG_OWNER, "Debugger.Source");
> 
> This is a bit of an aside/curiosity, because I agree that we should throw an
> error on principle if people are mixing up their Debugger objects, but do we
> actually rely on this property holding for the script query?

No, it should work fine --- but mixing objects from different Debuggers is usually a sign of confusion, so I think it's still a good idea to check for it.

Comment 6

[Jim Blandy :jimb]

Try push looks good.

Comment 7

[Jim Blandy :jimb]

Oh, let me update the patch posted to the bug...

Comment 8

[Jim Blandy :jimb]

Attachment: Make Debugger.prototype.findScripts validate query 'source' property. r=fitzgen

Updated per review comments.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f13cf40c6aeb
Make Debugger.prototype.findScripts validate query 'source' property. r=fitzgen

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f13cf40c6aeb

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f13cf40c6aeb