90597 - CACHED_SET/CACHED_GET refcount bug

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Mike Epstein]

The CACHED_SET and CACHED_GET macros have code like this:

    sprop->nrefs++;
    ok = SPROP_{G,S}ET(...);
    if (ok)
    {
        ...
        js_DropScopeProperty(..., sprop);
    }

if SPROP_{G,S}ET fails, js_DropScopeProperty is never called.  This causes that 
JSScopeProperty to have a one-too-high refcount, which results in two problems:

(a) The JSScopeProperty will never be freed, because it is only freed when its 
refcount drops to 0.  Hence memory leak.

(b) If the JSScopeProperty is in the property cache, after its refcount drops 
to 1 its "symbols" list is null, causing PROPERTY_CACHE_TEST to call something 
like sym_id, which derefs symbols and crashes.

I will attach a proposed patch shortly.

Comment 1

[Mike Epstein]

Attachment: Proposed patch

Comment 2

[Phil Schwartau]

cc'ing Brendan on this one!

Comment 3

[Brendan Eich [:brendan]]

Mine all mine.

/be

Comment 4

[Brendan Eich [:brendan]]

That patch is fine, apart from the discordant brace style.  Thanks, please feel
free to use it, or the style-conforming version of it I'm about to attach. 
Cc'ing reviewers, please do your things.

/be

Comment 5

[Brendan Eich [:brendan]]

Actually, my patch fixes a problem in the first patch: you must JS_LOCK_SCOPE
before calling js_DropScopeProperty.

/be

Comment 6

[Brendan Eich [:brendan]]

Attachment: better fix (locks scope and drops property no matter what SPROP_GET or SET returns, and unlocks scope after)

Comment 7

[Brendan Eich [:brendan]]

Attachment: diff -wu version of last patch

Comment 8

[Brendan Eich [:brendan]]

rogerl, how about r=?  jband, sr=?  shaver, for good measure.

/be

Comment 9

[rogerl (gone)]

I certainly don't know enough about this area. Is it really ok to continue after 
receiving a FALSE from SPROP_GET? I know we'll return the FALSE value soon 
enough but some significant (looking) code is executed first. Throughout the 
rest of the engine we interpret a FALSE value as a signal to bail out - as if an 
exception were thrown. Is it the case that, no matter what caused the initial 
failure, it's ok to continue with the rest of the sequence?

Comment 10

[Brendan Eich [:brendan]]

rogerl: ok is tested after each CACHED_GET or CACHED_SET call, and if false, we
goto out (possibly cleaning up anything that would leak otherwise).  The patch
must drop the found sprop and unlock the scope, however -- that's the bug being
fixed here.

/be

Comment 11

[Phil Schwartau]

Just ran the JS test suite on Linux with patch (id=42516) applied.
No new regressions introduced. All test failures are previously known,
and due to other bugs still being worked on -

Comment 12

[Brendan Eich [:brendan]]

hyatt says r/sr=hyatt, waterson says sr/r=waterson -- I'm so happy!

/be

Comment 13

[Phil Schwartau]

Marking Verified -

Comment 14

[Brendan Eich [:brendan]]

*** Bug 84720 has been marked as a duplicate of this bug. ***