754281 - MMGC_GCENTER's mechanism for determining stackEnter is fragile

Status: RESOLVED FIXED

(Tamarin Graveyard :: Garbage Collection (mmGC), defect)

Description

[Felix S. Klock II [:pnkfelix, :fklock]]

msong reports that the MMGC_GCENTER mechanism for determining stack boundaries is fragile, in that it is relying on a strict correspondence between stack layout and local variable declaration order.

As msong put it:
  Given local variables declared as

  Foo a;
  Bar b;

  It assumes that the address of |a| is greater than the address of |b|.
  I.e, &a > &b (on x86/x64, the stack grows down)
  It may always be true for some build configurations (compilers, compiler settings) but there is a case that is not a valid assumption.

  The following fails if the stack-order assumption that it relies on doesn't hold

  MMGC_GCENTER(...);
  GCRef<...> foo = ...;

----

The funny thing is, it *looks* like we supposedly identified and fixed this problem back in Bug 506013.

What happened here?

* Am I mistakenly conflating these two tickets?

* Or did we put in the primitive support for extracting the real stack pointer and then forget to adjust the MMGC_GCENTER mechanism?

* Or were we implicitly relying on treilly's statement from Bug 506013, comment 2 that "There are no known instances of this bug in the wild since usually there's interleaving frames between the MMGC_GCENTER and the use of stack-only referenced GC objects."

----

Anyway, we need to fix this by some means.

It sounds like switching GC::GetStackTop() to unconditionally employ AVMPI_getThreadStackBase() would fix the problem; so for the *short* term, we could do that for the targets where the compiler is violating the above assumption.

Long term, we should find a single strategy that works everywhere; that could mean switching to AVMPI_getThreadStackBase() everywhere (which may happen for other reasons anyway).  Or it could mean something else (e.g. not exposing gc-references to arbitrary parts of the native stack).

Comment 1

[Felix S. Klock II [:pnkfelix, :fklock]]

StackOverflow points out one workaround I had not considered: put all of the declarations that you want to order into a struct:

  http://stackoverflow.com/a/1102165/36585

Comment 2

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Felix S Klock II from comment #1)
> StackOverflow points out one workaround I had not considered: put all of the
> declarations that you want to order into a struct:
> 
>   http://stackoverflow.com/a/1102165/36585

(This would not be guaranteed to deal with temps that had been spilled to the stack, though.  There probably is no silver bullet here.)

Comment 3

[Felix S. Klock II [:pnkfelix, :fklock]]

Adobe people can also see further discussion here: 

  https://zerowing.corp.adobe.com/x/-BBjK

Comment 4

[Felix S. Klock II [:pnkfelix, :fklock]]

Spawned off subtask (performance analysis of switching to AVMPI_getThreadStackBase) in Bug 759738.

Comment 5

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch G v1: guard use of GetStackEnter in spot where accuracy matters for soundness

patch G v1: guard use of GetStackEnter in spot where accuracy matters for soundness

Comment 6

[Tony Printezis]

Felix, the changes look good. I'd like to only make a stylistic suggestion (and I don't know if approach is used in avmplus; feel free to ignore it):

If MMGC_HAS_TRUSTWORTHY_GET_STACK_ENTER was defined to 0 or 1, this

#ifdef MMGC_HAS_TRUSTWORTHY_GET_STACK_ENTER
         // temporary crutch until we're moved over to the MMGC_GCENTER system
         if(stackEnter == NULL)
             return AVMPI_getThreadStackBase();
         return GetStackEnter();
#else
        return AVMPI_getThreadStackBase();
#endif // MMGC_HAS_TRUSTWORTHY_GET_STACK_ENTER

can be rewritten as:

if (MMGC_HAS_TRUSTWORTHY_GET_STACK_ENTER && stackEnter != NULL) {
    return GetStackEnter();
else {
    return AVMPI_getThreadStackBase();
}

which is more concise and more readable (IMHO of course).

Comment 7

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Tony Printezis from comment #6)
> Felix, the changes look good. I'd like to only make a stylistic suggestion
> (and I don't know if approach is used in avmplus; feel free to ignore it):

FYI: That's not the style we tend to use (in the AVM nor in the Flash Runtime).  Usually C preprocessor symbols that are guarding features are not intermixed with C/C++ source expressions, and instead are solely used to direct the preprocessor.

One motivation for this that I can think of off-hand is that IDE's can efficiently do a good job of coloring the source based on the static setting of the C preprocessor symbols.

Comment 8

[Tony Printezis]

(In reply to Felix S Klock II from comment #7)
> (In reply to Tony Printezis from comment #6)
> > Felix, the changes look good. I'd like to only make a stylistic suggestion
> > (and I don't know if approach is used in avmplus; feel free to ignore it):
> 
> FYI: That's not the style we tend to use (in the AVM nor in the Flash
> Runtime).

bool has_trustworthy_get_stack_enter() {
#ifdef MMGC_HAS_TRUSTWORTHY_GET_STACK_ENTER
  return true;
#else // MMGC_HAS_TRUSTWORTHY_GET_STACK_ENTER
  return false;
#endif // MMGC_HAS_TRUSTWORTHY_GET_STACK_ENTER
}

if (has_trustworthy_get_stack_enter() && stackEnter != NULL) {
    return GetStackEnter();
} else {
    return AVMPI_getThreadStackBase();
}

?

I'm over-reacting a bit here and in this case it doesn't really matter. But I've seen some code where the body of a method is a sea of #ifdef's and it's just impossible to read and understand.

Comment 9

[Paul Hohensee]

Comment on attachment 636847 [details] [diff] [review]
patch G v1: guard use of GetStackEnter in spot where accuracy matters for soundness

Review of attachment 636847 [details] [diff] [review]:
-----------------------------------------------------------------

+1 for Tony's suggestion.  You'll want to change GC.h to refer to has_trustworthy_stack_enter() rather than GetStackTop().

Comment 10

[Felix S. Klock II [:pnkfelix, :fklock]]

<shrug>

I've already pushed the original version.  I won't veto someone else making Tony's change, but I think you both well know I have higher priority things to address.

Comment 11

[Paul Hohensee]

Comment on attachment 636847 [details] [diff] [review]
patch G v1: guard use of GetStackEnter in spot where accuracy matters for soundness

Review of attachment 636847 [details] [diff] [review]:
-----------------------------------------------------------------

Hadn't realized the fix had already been pushed.  My apologies.

Comment 12

[Tamarin Bot]

changeset: 7447:bf043aa12778
user:      Felix Klock II <[e-mail]>
summary:   Bug 754281: add preprocessor sym guarding GetStackEnter call (r=fklockii).

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