From d64bcc763a26684e3dc0fce7ab994f5b916256ae Mon Sep 17 00:00:00 2001 From: David Anderson Date: Thu, 9 Apr 2015 22:29:34 -0400 Subject: [PATCH] Fix bug where complex |this| values could be corrupted while evaluating function arguments. (bug 6329) --- sourcepawn/compiler/sc.h | 18 +++ sourcepawn/compiler/sc3.cpp | 139 +++++++++++++++--- .../tests/disabled/this-eval-ordering.sp | 32 ++++ 3 files changed, 172 insertions(+), 17 deletions(-) create mode 100644 sourcepawn/compiler/tests/disabled/this-eval-ordering.sp diff --git a/sourcepawn/compiler/sc.h b/sourcepawn/compiler/sc.h index 9399ed47..a5d959ab 100644 --- a/sourcepawn/compiler/sc.h +++ b/sourcepawn/compiler/sc.h @@ -248,6 +248,20 @@ typedef struct value_s { char boolresult; /* boolean result for relational operators */ cell *arrayidx; /* last used array indices, for checking self assignment */ + // Returns whether the value can be rematerialized based on static + // information, or whether it is the result of an expression. + bool canRematerialize() const { + switch (ident) { + case iVARIABLE: + case iCONSTEXPR: + return true; + case iREFERENCE: + return sym->vclass == sLOCAL; + default: + return false; + } + } + /* when ident == iACCESSOR */ struct methodmap_method_s *accessor; } value; @@ -256,6 +270,10 @@ typedef struct value_s { typedef struct svalue_s { value val; int lvalue; + + bool canRematerialize() const { + return val.canRematerialize(); + } } svalue; #define DECLFLAG_ARGUMENT 0x02 // The declaration is for an argument. diff --git a/sourcepawn/compiler/sc3.cpp b/sourcepawn/compiler/sc3.cpp index 7b733c19..92a48bf5 100644 --- a/sourcepawn/compiler/sc3.cpp +++ b/sourcepawn/compiler/sc3.cpp @@ -2662,7 +2662,7 @@ enum { * Generates code to call a function. This routine handles default arguments * and positional as well as named parameters. */ -static void callfunction(symbol *sym, const svalue *implicitthis, value *lval_result, int matchparanthesis) +static void callfunction(symbol *sym, const svalue *aImplicitThis, value *lval_result, int matchparanthesis) { static long nest_stkusage=0L; static int nesting=0; @@ -2680,7 +2680,7 @@ static int nesting=0; symbol *symret; cell lexval; char *lexstr; - int pending_this = (implicitthis ? TRUE : FALSE); + bool pending_this = !!aImplicitThis; assert(sym!=NULL); lval_result->ident=iEXPRESSION; /* preset, may be changed later */ @@ -2718,6 +2718,77 @@ static int nesting=0; error(234,sym->name,ptr); /* deprecated (probably a native function) */ } /* if */ + svalue implicit_this; + bool has_complex_this = false; + if (aImplicitThis) { + implicit_this = *aImplicitThis; + has_complex_this = !implicit_this.canRematerialize(); + } + + // If we have an implicit |this|, and it requires evaluating an expression + // to compute, then we have to save PRI across each argument. The reason is + // that Pawn will evaluate arguments like so: + // + // x[5].z(1, 2) + // + // Resolves to: + // + // begin: + // load.s.pri x + // add.pri.c 5 * sizeof(cell) + // marked: + // push.pri ; this + // const.pri 1 + // push.pri + // const.pri 2 + // push.pri 2 + // finished: + // push.c 3 ; argc + // + // The expressions from "marked" to "finished" are reversed, so that + // arguments appear in order. Thus the final sequence is: + // + // begin: + // load.s.pri x + // add.pri.c 5 * sizeof(cell) + // marked: + // const.pri 2 + // push.pri 2 + // const.pri 1 + // push.pri + // push.pri ; this + // finished: + // push.c 3 ; argc + // + // To account for this, if |this| requires evaluation, we evaluate it ahead + // of time and store it on the stack, so we can extract it again later. + // Because there is no opcode to do this, we have to play hot potato like so: + // + // begin: + // load.s.pri x + // add.pri.c 5 * sizeof(cell) + // marked: + // const.pri 2 + // pop.alt ; pop |this| + // push.pri 2 + // push.alt ; re-push |this| + // const.pri 1 + // pop.alt ; pop |this| + // push.pri + // push.alt ; re-push |this| + // finished: + // push.c 3 ; argc + // + if (has_complex_this) { + // Compute an rvalue of |implicit_this|. Note that if it's an accessor, + // this reduces it to an iEXPRESSION. That's ok. We don't touch it + // otherwise though, so the type checking logic below basically acts the + // same. We are careful to not double-evaluate however. + rvalue(&implicit_this.val); + pushreg(sPRI); + nest_stkusage++; + } + /* run through the arguments */ arg=sym->dim.arglist; assert(arg!=NULL); @@ -2788,8 +2859,8 @@ static int nesting=0; if (arg[argidx].ident!=0 && arg[argidx].numtags==1) /* set the expected tag, if any */ lval.cmptag=arg[argidx].tags[0]; if (pending_this) { - lval = implicitthis->val; - lvalue = implicitthis->lvalue; + lval = implicit_this.val; + lvalue = implicit_this.lvalue; } else { lvalue = hier14(&lval); if (lvalue && lval.ident == iACCESSOR) { @@ -2801,7 +2872,7 @@ static int nesting=0; switch (arg[argidx].ident) { case 0: /* On the first pass, we donm't have all of the parameter info. - * Hpwever, use information must be marked anyway, otherwise vars + * However, use information must be marked anyway, otherwise vars * declared previously will be omitted in the second psas. See * SourceMod bug 4643. */ @@ -2810,6 +2881,10 @@ static int nesting=0; markusage(lval.sym, uREAD); break; case iVARARGS: + // hier13() should filter non-methodmap functions, and |this| is + // always an iVARIABLE. + assert(!pending_this); + /* always pass by reference */ if (lval.ident==iVARIABLE || lval.ident==iREFERENCE) { assert(lval.sym!=NULL); @@ -2851,11 +2926,20 @@ static int nesting=0; if (lval.ident==iLABEL || lval.ident==iFUNCTN || lval.ident==iREFFUNC || lval.ident==iARRAY || lval.ident==iREFARRAY) error(35,argidx+1); /* argument type mismatch */ - if (lvalue) - rvalue(&lval); /* get value (direct or indirect) */ + + if (lvalue) { + // Note: do not load anything if the implicit this was pre-evaluted + // for being too complex. + if (!(pending_this && has_complex_this)) + rvalue(&lval); /* get value (direct or indirect) */ + } + /* otherwise, the expression result is already in PRI */ assert(arg[argidx].numtags>0); - check_userop(NULL,lval.tag,arg[argidx].tags[0],2,NULL,&lval.tag); + + // Do not allow user operators to transform |this|. + if (!pending_this) + check_userop(NULL,lval.tag,arg[argidx].tags[0],2,NULL,&lval.tag); if (!checktags_string(arg[argidx].tags, arg[argidx].numtags, &lval)) checktag(arg[argidx].tags, arg[argidx].numtags, lval.tag); if (lval.tag!=0) @@ -2863,6 +2947,10 @@ static int nesting=0; argidx++; /* argument done */ break; case iREFERENCE: + // hier13() should filter non-methodmap functions, and |this| is + // always an iVARIABLE. + assert(!pending_this); + if (!lvalue) error(35,argidx+1); /* argument type mismatch */ if (lval.sym!=NULL && (lval.sym->usage & uCONST)!=0 && (arg[argidx].usage & uCONST)==0) @@ -2886,6 +2974,10 @@ static int nesting=0; markusage(lval.sym,uWRITTEN); break; case iREFARRAY: + // hier13() should filter non-methodmap functions, and |this| is + // always an iVARIABLE. + assert(!pending_this); + if (lval.ident!=iARRAY && lval.ident!=iREFARRAY && lval.ident!=iARRAYCELL && lval.ident!=iARRAYCHAR) { @@ -2971,19 +3063,31 @@ static int nesting=0; argidx++; /* argument done */ break; } /* switch */ - pushreg(sPRI); /* store the function argument on the stack */ - markexpr(sPARM,NULL,0); /* mark the end of a sub-expression */ - nest_stkusage++; + + // If we are processing |this| and it's complex, then it was never + // actually evaluated as an argument, so don't push it. + if (!(pending_this && has_complex_this)) { + // If we have a complex |this|, pop it as ALT, push the evaluated + // argument, then push |this| again. This makes sure |this| is + // always the last argument. + if (has_complex_this) { + popreg(sALT); + pushreg(sPRI); /* store the function argument on the stack */ + pushreg(sALT); + } else { + pushreg(sPRI); /* store the function argument on the stack */ + } + markexpr(sPARM,NULL,0); /* mark the end of a sub-expression */ + nest_stkusage++; + } } /* if */ assert(arglist[argpos]!=ARG_UNHANDLED); nargs++; - /** - * We can already have decided it was time to close because of pending_this. - * If that's the case, then bail out now. - */ + // We can already have decided it was time to close because of pending_this. + // If that's the case, then bail out now. if (pending_this && close) { - pending_this = FALSE; + pending_this = false; break; } @@ -3002,7 +3106,7 @@ static int nesting=0; } /* if */ } /* if */ - pending_this = FALSE; + pending_this = false; } while (!close && freading && !matchtoken(tENDEXPR)); /* do */ } /* if */ /* check remaining function arguments (they may have default values) */ @@ -3057,6 +3161,7 @@ static int nesting=0; arglist[argidx]=ARG_DONE; } /* for */ stgmark(sENDREORDER); /* mark end of reversed evaluation */ + pushval((cell)nargs /* *sizeof(cell)*/ ); nest_stkusage++; ffcall(sym,NULL,nargs); diff --git a/sourcepawn/compiler/tests/disabled/this-eval-ordering.sp b/sourcepawn/compiler/tests/disabled/this-eval-ordering.sp new file mode 100644 index 00000000..fd0911c2 --- /dev/null +++ b/sourcepawn/compiler/tests/disabled/this-eval-ordering.sp @@ -0,0 +1,32 @@ +native void printnum(int x); + +methodmap X +{ + public void GetValue() { + printnum(view_as(this)); + } + public void SetValue(const char[] key, any value) { + printnum(view_as(this)); + } +}; + +int Sandwich(const char[] value) +{ + return 999; +} + +////////////////////////////////// + +X gTargets[] = { + view_as(10), + view_as(20), + view_as(30), +} + +// Should print 30 three times. +public main() +{ + printnum(view_as(gTargets[2])); + gTargets[2].GetValue(); + gTargets[2].SetValue("TargetBanTime", Sandwich("hello")); +}