ScriptRunner: ASSERT -> RELEASE_ASSERT.

ScriptRunner: ASSERT -> RELEASE_ASSERT_WITH_SECURITY_IMPLICATION.

We've had a couple of corner case bugs where some elaborately malicious script
moving behavior will confuse ScriptRunner & upper layers. In those cases, a
ScriptLoader is assocated w/ ScriptRunner SR1 whereas we expect it to be
associated w/ ScriptRunner SR2. SR2 will never notice anything, the ScriptLoader
is destroyed but SR1 still refers to it, and when SR1 is destroyed, it will
cause a use-after-free when it tries to detach the ScriptLoader.

Normal pages should never bump into this.

R=sigbjornf@opera.com, haraken@chromium.org
BUG=460426
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=190600

[marja, 2015-02-20 10:25:44]

ptal

I suggest we try this for a while in M42, then see whether it's stable enough
(no worrying amount of crash reports), and then maybe merge back to M41 too.

[sof, 2015-02-20 10:56:52]

lgtm, but might the stronger RELEASE_ASSERT() be in order?

[marja, 2015-02-20 11:03:47]

On 2015/02/20 10:56:52, sof wrote:
> lgtm, but might the stronger RELEASE_ASSERT() be in order?

Hmm, what do you mean stronger?

Afaics, in the non-asan case:

#define RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(assertion)
RELEASE_ASSERT(assertion)

... so seems to just be the name which is different, telling whoever bumps into
this to file a security bug.

[sof, 2015-02-20 11:10:01]

On 2015/02/20 11:03:47, marja wrote:
> On 2015/02/20 10:56:52, sof wrote:
> > lgtm, but might the stronger RELEASE_ASSERT() be in order?
> 
> Hmm, what do you mean stronger?
> 
> Afaics, in the non-asan case:
> 
> #define RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(assertion)
> RELEASE_ASSERT(assertion)
> 
> ... so seems to just be the name which is different, telling whoever bumps
into
> this to file a security bug.

Per documentation, RELEASE_ASSERT() indicates a definite security vuln,
RELEASE_ASSERT_WITH_SECURITY_IMPLICATION() a potential one.

[marja, 2015-02-20 11:12:50]

On 2015/02/20 11:10:01, sof wrote:
> On 2015/02/20 11:03:47, marja wrote:
> > On 2015/02/20 10:56:52, sof wrote:
> > > lgtm, but might the stronger RELEASE_ASSERT() be in order?
> > 
> > Hmm, what do you mean stronger?
> > 
> > Afaics, in the non-asan case:
> > 
> > #define RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(assertion)
> > RELEASE_ASSERT(assertion)
> > 
> > ... so seems to just be the name which is different, telling whoever bumps
> into
> > this to file a security bug.
> 
> Per documentation, RELEASE_ASSERT() indicates a definite security vuln,
> RELEASE_ASSERT_WITH_SECURITY_IMPLICATION() a potential one.

Ah, you're right, I somehow missed that.

However, the doc of RELEASE_ASSERT_WITH_SECURITY_IMPLICATION also says:

   Use in places where failure of the assertion indicates a possible security
   vulnerability. Classes of these vulnerabilities include bad casts, out of
   bounds accesses, use-after-frees, etc. Please be sure to file bugs for these
   failures using the security template:

And the failure in this case is use after free which is listed there, so I think
it's suitable for this.

[sof, 2015-02-20 12:05:16]

On 2015/02/20 11:12:50, marja wrote:
> On 2015/02/20 11:10:01, sof wrote:
> > On 2015/02/20 11:03:47, marja wrote:
> > > On 2015/02/20 10:56:52, sof wrote:
> > > > lgtm, but might the stronger RELEASE_ASSERT() be in order?
> > > 
> > > Hmm, what do you mean stronger?
> > > 
> > > Afaics, in the non-asan case:
> > > 
> > > #define RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(assertion)
> > > RELEASE_ASSERT(assertion)
> > > 
> > > ... so seems to just be the name which is different, telling whoever bumps
> > into
> > > this to file a security bug.
> > 
> > Per documentation, RELEASE_ASSERT() indicates a definite security vuln,
> > RELEASE_ASSERT_WITH_SECURITY_IMPLICATION() a potential one.
> 
> Ah, you're right, I somehow missed that.
> 
> However, the doc of RELEASE_ASSERT_WITH_SECURITY_IMPLICATION also says:
> 
>    Use in places where failure of the assertion indicates a possible security
>    vulnerability. Classes of these vulnerabilities include bad casts, out of
>    bounds accesses, use-after-frees, etc. Please be sure to file bugs for
these
>    failures using the security template:
> 
> And the failure in this case is use after free which is listed there, so I
think
> it's suitable for this.

alright, that works for me.

[haraken, 2015-02-20 20:14:20]

LGTM

[haraken, 2015-02-20 20:14:24]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2015-02-20 20:15:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/936333005/20001

[commit-bot: I haz the power, 2015-02-21 00:32:33]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=190600