546801 - StringIndexer is not safe

Status: VERIFIED FIXED

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

Description

[Steven Johnson]

StringIndexer caches the secret pointers in a String in the name of speed; this was a valid assumption at one time, but the existence of convertToDynamic() invalidates this assumption. StringIndexer will probably have to go away, or become thin syntactic sugar around String::charAt.

Imagine code like:

  StringIndexer foo(s);
  DoSomethingThatCanTriggerGC();

at this point, s might have been converted to a dynamic string, but foo still points to the old, discarded memory.

Comment 1

[Tommy Reilly]

Doesn't the GC see the pointers in foo and keep the old buffer's around?  Its not like convertToDynamic is explicitly deleting them.  Keep in mind the GC allows interior pointers to exist on the stack and it will find them if that's the issue.

Comment 2

[Steven Johnson]

(In reply to comment #1)
> Doesn't the GC see the pointers in foo and keep the old buffer's around?  

This is true; I had forgotten we handle interior pointers now. The genesis of this bug is a very-hard-to-repro crash that (so far) this is the most likely explanation for: the StringIndexer embedded in XMLParser has a String that is still valid and plausible, but the Pointers field of StringIndexer doesn't match the values in the String (as it should) and is not valid memory (though the pointers themselves look reasonable, so a possible cause is that it's memory that was decommitted).

If GC could somehow miss this interior pointer, it would explain this behavior. Are we certain that the interior-pointer code is 100% fully baked at this point?

Comment 3

[Lars T Hansen]

(In reply to comment #2)

> If GC could somehow miss this interior pointer, it would explain this behavior.
> Are we certain that the interior-pointer code is 100% fully baked at this
> point?

Not really, because we're not relying on it on most platforms.  Also the pointer must be interior, can't point before the start or after then end.  (Compilers have been known to do that in the past, but I think it's considered bad practice.)

Comment 4

[Tommy Reilly]

To add to Lar's comment there was a bug we fixed last week where the scan interior pointers functionality wasn't being propagated to stack segments.  Has the bug been repro'd since last weeks tamarin integration?

Comment 5

[Steven Johnson]

Yeah, after looking at the comments and further investigation, I think this is a red herring -- I don't think StringIndexer is unsafe in its current incarnation, and likely not the cause of the crash I'm investigating. Withdrawing.

Comment 6

[Steven Johnson]

Reopening -- new evidence suggests this really is an issue after all. In particular, Flash *doesn't* use GC memory to store SWF, thus when an ABC buffer is released, the old pointer could well become invalid as it's outside of the GC world.

Comment 7

[Steven Johnson]

Attachment: Patch

Turn StringIndexer into a simple wrapper around charAt. This is suboptimal, but the only safe way to implement it in the general case, given constant-pool-string dynamicizing.

Comment 8

[Tommy Reilly]

(In reply to comment #6)
> Reopening -- new evidence suggests this really is an issue after all. In
> particular, Flash *doesn't* use GC memory to store SWF, thus when an ABC buffer
> is released, the old pointer could well become invalid as it's outside of the
> GC world.

This isn't entirely true, the swf buffer is unmanaged memory but its only ever deleted from the dtor of a GCFinalizedObject.

Comment 9

[Edwin Smith]

Comment on attachment 428592 [details] [diff] [review]
Patch

seems safe, of course, but unknown performance implications.

Comment 10

[Steven Johnson]

(In reply to comment #8)
> This isn't entirely true, the swf buffer is unmanaged memory but its only ever
> deleted from the dtor of a GCFinalizedObject.

Right, and that's the issue... in the example I gave above, if the collection is triggered, and collects+finalizes the object that is managing the unmanaged memory, we're still dead.(In reply to comment #9)

> (From update of attachment 428592 [details] [diff] [review])
> seems safe, of course, but unknown performance implications.

Agreed -- of course, Crowder's Law applies, but we should still run perf tests before landing so we know what the impact is.

Comment 11

[Steven Johnson]

http://hg.mozilla.org/tamarin-redux/rev/f802b7878d31

Comment 12

[Brent Baker]

This patch appears to have fixed another linux specific issue, bug# 402864. I have commented in the bug referencing this issue.

Comment 13

[Chris Peyer]

verified in argo changeset 3749	f802b7878d31