1537656 - MOZ_CAN_RUN_SCRIPT analysis should disallow reassigning arguments

Status: NEW

(Developer Infrastructure :: Source Code Analysis, enhancement, P3)

Description

[Boris Zbarsky [:bzbarsky]]

The analysis assumes that arguments are recursively live because the caller keeps them alive. But that means that this code passes the analysis:

  MOZ_CAN_RUN_SCRIPT void test_arg_reassignment_1(RefCountedBase* arg) {
    arg = new RefCountedBase();
    test2(arg);
  }

where test2 is a MOZ_CAN_RUN_SCRIPT function. More to the point, if you have a MOZ_CAN_RUN_SCRIPT function that takes an nsINode* aNode and then walks the node tree while assigning the currently-walked-to thing to aNode, then aNode is no longer guaranteed live.

We should either disallow reassigning arguments in MOZ_CAN_RUN_SCRIPT functions (both via operator= and taking references or pointers to them, probably) or somehow detect the cases when they're reassigned.

Not sure whether this should be part of the can-run-script analysis or a separate analysis...

Comment 1

[Boris Zbarsky [:bzbarsky]]

So I tried the obvious thing: detecting cases when the argument of a MOZ_CAN_RUN_SCRIPT function is assigned to and disallowing that. This definitely has undesirable false positives. For example nsContentUtils::DispatchInputEvent assigns to aTextEditor but then does not actually pass it to any MOZ_CAN_RUN_SCRIPT functions, so it's not actually a problem that it does the assignment.

I am vaguely trying to figure out how to write an analysis which will look at the actual args passed to MOZ_CAN_RUN_SCRIPT functions, then see whether any of those are caller params that are reassigned.

Ideally, of course, we would be doing this on some sort of SSA data structure, not an AST, so reassignment would be handled automatically... :(

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Non-working patch as described in comment 1