Bug 101940 – StructureStubInfo should NOT be movable

• Status: RESOLVED
• Resolution: INVALID
• Component: JavaScriptCore

[Yong Li]

Currently CodeBlock contains a Vector of StructureStubInfo, which means those StructureStubInfo objects can be moved whenever Vector wants to move them. A potential issue is StructureStubInfo::watchpoints holds the address of the owner StructureStubInfo. So it could end up with dangling pointer issue.

I haven't seen this really happens though, probably because the Vector grows and shrinkToFits always before any watchpoint is added?

[Yong Li]

Created attachment 173691 [details]
The proposed patch

[Filip Pizlo]

Comment on attachment 173691 [details]
The proposed patch

Wouldn't it be easier to just use a SegmentedVector?

[Yong Li]

(In reply to comment #2)
> (From update of attachment 173691 [details])
> Wouldn't it be easier to just use a SegmentedVector?

yeah. I just saw m_llintCallLinkInfos is a SegmentedVector. But probably I would have to leave StructureStubInfo copyable otherwise it won't build. I'm trying to go through similar issues, and I noticed that this one is probably unsafe, too:

Vector<GlobalResolveInfo> m_globalResolveInfos;

It seems a GlobalResolveInfo's address can be hard-coded in JIT executable. This might explain an occasional crash I saw where a JIT executable read garbage from a hard-coded address. Will post another patch after going through these Vectors

[Filip Pizlo]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 173691 [details] [details])
> > Wouldn't it be easier to just use a SegmentedVector?
> 
> yeah. I just saw m_llintCallLinkInfos is a SegmentedVector. But probably I would have to leave StructureStubInfo copyable otherwise it won't build. I'm trying to go through similar issues, and I noticed that this one is probably unsafe, too:
> 
> Vector<GlobalResolveInfo> m_globalResolveInfos;
> 
> It seems a GlobalResolveInfo's address can be hard-coded in JIT executable. This might explain an occasional crash I saw where a JIT executable read garbage from a hard-coded address. Will post another patch after going through these Vectors

Actually, that reminds me.  This is all safe because we only grab the addresses of StructureStubInfo's and GlobalResolveInfo's (and all of those others) after we're done appending (and resizing) the relevant vectors.

So, unless you can find an example where we append after emitting code (and creating watchpoints, and doing other things that grab pointers to those vector elements), I'm going to close as Resolved/Invalid.

[Yong Li]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > (From update of attachment 173691 [details] [details] [details])
> > > Wouldn't it be easier to just use a SegmentedVector?
> > 
> > yeah. I just saw m_llintCallLinkInfos is a SegmentedVector. But probably I would have to leave StructureStubInfo copyable otherwise it won't build. I'm trying to go through similar issues, and I noticed that this one is probably unsafe, too:
> > 
> > Vector<GlobalResolveInfo> m_globalResolveInfos;
> > 
> > It seems a GlobalResolveInfo's address can be hard-coded in JIT executable. This might explain an occasional crash I saw where a JIT executable read garbage from a hard-coded address. Will post another patch after going through these Vectors
> 
> Actually, that reminds me.  This is all safe because we only grab the addresses of StructureStubInfo's and GlobalResolveInfo's (and all of those others) after we're done appending (and resizing) the relevant vectors.
> 
> So, unless you can find an example where we append after emitting code (and creating watchpoints, and doing other things that grab pointers to those vector elements), I'm going to close as Resolved/Invalid.

One example: When the stub op_get_by_id_self_fail is called (JITStubs.cpp), it can call JIT::compileGetByIdSelfList, which can finally reach codeBlock->shrinkToFit(CodeBlock::LateShrink)

I see CodeBlock::shrinkToFit only shrinks m_globalResolveInfos for EarlyShrink, but it shrinks other vectors like m_structureStubInfos.

Should we worry about that?

It is probably still safe assuming shrinkToFit(EarlyShrink) must have been called before shrinkToFit(LateShrink), and there is no new stubInfo inserted since shrinkToFit(EarlyShrink). However, I still feel this is too risky...

[Filip Pizlo]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (In reply to comment #2)
> > > > (From update of attachment 173691 [details] [details] [details] [details])
> > > > Wouldn't it be easier to just use a SegmentedVector?
> > > 
> > > yeah. I just saw m_llintCallLinkInfos is a SegmentedVector. But probably I would have to leave StructureStubInfo copyable otherwise it won't build. I'm trying to go through similar issues, and I noticed that this one is probably unsafe, too:
> > > 
> > > Vector<GlobalResolveInfo> m_globalResolveInfos;
> > > 
> > > It seems a GlobalResolveInfo's address can be hard-coded in JIT executable. This might explain an occasional crash I saw where a JIT executable read garbage from a hard-coded address. Will post another patch after going through these Vectors
> > 
> > Actually, that reminds me.  This is all safe because we only grab the addresses of StructureStubInfo's and GlobalResolveInfo's (and all of those others) after we're done appending (and resizing) the relevant vectors.
> > 
> > So, unless you can find an example where we append after emitting code (and creating watchpoints, and doing other things that grab pointers to those vector elements), I'm going to close as Resolved/Invalid.
> 
> One example: When the stub op_get_by_id_self_fail is called (JITStubs.cpp), it can call JIT::compileGetByIdSelfList, which can finally reach codeBlock->shrinkToFit(CodeBlock::LateShrink)

How does it end up calling shrinkToFit()?

> 
> I see CodeBlock::shrinkToFit only shrinks m_globalResolveInfos for EarlyShrink, but it shrinks other vectors like m_structureStubInfos.

It should probably only shrink m_structureStubInfos in an EarlyShrink, but...

> 
> Should we worry about that?

... it shouldn't be a problem, since shrinking a vector that has already been shrunk without adding things to it is a no-op.

> 
> It is probably still safe assuming shrinkToFit(EarlyShrink) must have been called before shrinkToFit(LateShrink), and there is no new stubInfo inserted since shrinkToFit(EarlyShrink). However, I still feel this is too risky...

Yeah.