548175 - String::Pointers is not safe to use across a GC collection, but String does anyway

Status: VERIFIED FIXED

(Tamarin Graveyard :: Virtual Machine, defect, P2)

Description

[Steven Johnson]

The pointers in this struct could be invalidated by a GC collection: if the string was a static string pointing into an ABC's constant pool, and the ScriptBuffer containing the ABC is collected, and is allocated via non-GC memory, we'll point to bogus (possibly decommitted) memory.

We need to audit the usage of String::Pointers to ensure we don't use it across such a boundary.

Comment 1

[Steven Johnson]

Attachment: Patch

Comment 2

[Steven Johnson]

Attachment: Patch v2

Krzysztof points out that with this patch, internSubstring can now produce a dependent string, which is undesirable in this case: a tiny substring could force a giant XML string to be held in memory. Revised patch simply calls fixDependentString() after substring(), which should do the prudent thing.

Comment 3

[Krzysztof Palacz]

Comment on attachment 428791 [details] [diff] [review]
Patch v2

As a general comment, fixing this bug required looking into all the functions recursively called from inside every Pointers live range, and checking that no GC allocation can occur. As the code evolves, functions that didn't allocate before may start allocating, and the bug may reappear. 

A convention to distinguish recursively allocating functions from non-allocating functions would be helpful. One extreme idea would be require an explicit GC instance to be passed as the last argument to every potentially allocating function (that would make everyone think twice about adding an allocation where there wasn't any before!). It sounds painful, but I've seen a variant of it in the wild, so it's feasible (and things will only get worse if not only strings but other types of objects start moving around).

Comment 4

[Edwin Smith]

Comment on attachment 428791 [details] [diff] [review]
Patch v2

we're eons from "obviously no bugs" and into solid "no obvious bugs" territory.

Comment 5

[Steven Johnson]

http://hg.mozilla.org/tamarin-redux/rev/27233799b1b2

Comment 6

[Chris Peyer]

Verified in argo changeset 3750	27233799b1b2