Last Comment Bug 644250 - fix Apple gcc bug caused by -fstack-protector
: fix Apple gcc bug caused by -fstack-protector
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All Mac OS X
: -- normal (vote)
: ---
Assigned To: general
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-03-23 11:19 PDT by Luke Wagner [:luke]
Modified: 2011-04-12 09:30 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments

Description Luke Wagner [:luke] 2011-03-23 11:19:30 PDT
http://hg.mozilla.org/tracemonkey/rev/4290338c3956 caused a bunch of orange on OSX10.5 (and a bit on 10.6).  Tracking it down fingered bad code generated by the -fstack-protector option which is on by default.  To fix the burning I landed a simple one-liner (http://hg.mozilla.org/tracemonkey/rev/663a3afb238e) to set -fno-stack-protector (all platforms).  As an experiment, I also took out the hack gal had to add for a similar-sounding Apple gcc bug (http://hg.mozilla.org/tracemonkey/rev/a110692f5d98) and so far its all green (modulo random orange) on OS X on tbpl.

My goal with the bug is to:
 1. explain why its -fstack-protector's fault (below)
 2. tell someone in security since technically I am removing a security feature (only from js/src, where I have personally never seen this feature prevent an existing bug from being exploitable).
 3. get expert advice as to where -fno-stack-protector should actually go (e.g., to limit it to Apple gcc)

-fstack-protector is described in http://reverseshell.wordpress.com/2010/10/06/gcc-stack-protection-introduction, but its basically what you'd expect: a canary value on the stack in front of the return address that gets checked at the end.

The *** line is the crash address:

http://hg.mozilla.org/tracemonkey/file/a110692f5d98/js/src/jsinterp.cpp#l6941
0x3c23151   call   0x3d2a480 <AbortProfiling>
0x3c23156   xor    %edx,%edx
0x3c23158   test   %ebx,%ebx            // ebx must hold interpReturnOK
0x3c2315a   setne  %dl                  // stick bool(interpReturnOK) in edx
0x3c2315d   mov    0xb4(%esp),%edi      // ~InterpExitGuard() {
0x3c23164   decl   0xbc(%edi)
0x3c2316a   mov    0x75c(%esp),%eax
0x3c23171   mov    0xac(%esp),%ebp
0x3c23178   mov    %eax,0x0(%ebp)
0x3c2317b   mov    0x760(%esp),%eax
0x3c23182   mov    %eax,0x4(%ebp)
0x3c23185   mov    0x764(%esp),%eax
0x3c2318c   mov    %eax,0x8(%ebp)
0x3c2318f   mov    %ebp,0x40(%edi)      // }
0x3c23192   mov    0x6a4(%esp),%eax     // ~JSAutoResolveFlags() {
0x3c23199   mov    %eax,0xe8(%edi)      // }
0x3c2319f   mov    %edx,%eax            // move bool(interpReturnOK) in eax
0x3c231a1   mov    0x4ee111(%ebx),%edx  // wtf, edx is the bool interpReturnOK!
                                        // however *(0x4ee112) == 0 so no crash
0x3c231a7   mov    0x93c(%esp),%ecx
0x3c231ae   xor    (%edx),%ecx          // *** crash: $edx = 0
0x3c231b0   jne    0x3c2eef0            // jump to dyld_stub___stack_chk_fail
0x3c231b6   add    $0x94c,%esp
// js::Interpret epilogue

So it seems clear that somehow -fstack-protector is mistaken about $ebx.
Comment 1 Ted Mielczarek [:ted.mielczarek] 2011-03-23 11:27:04 PDT
This sounds very similar to the buffer security checks in VC++, which the security group was very excited about:
http://msdn.microsoft.com/en-us/library/8dbf701c%28v=VS.80%29.aspx
Comment 2 Luke Wagner [:luke] 2011-03-24 14:47:07 PDT
Nomis101: not sure why you changed OS... wrong bug?
Comment 3 Nomis101 2011-03-24 15:01:31 PDT
Sorry, I didn't changed OS by purpose, I only CCed me.
Comment 4 Luke Wagner [:luke] 2011-03-29 11:40:46 PDT
Ted: so, would you make any changes to http://hg.mozilla.org/tracemonkey/rev/663a3afb238e (other than removing -fno-stack-protector, of course :).
Comment 5 Luke Wagner [:luke] 2011-04-11 10:47:49 PDT
Ted: should I fix anything or can I just wontfix this?
Comment 6 Ted Mielczarek [:ted.mielczarek] 2011-04-11 11:39:57 PDT
Sorry, I don't know enough about GCC internals.
Comment 7 Luke Wagner [:luke] 2011-04-11 13:40:51 PDT
Oh, I was really just asking about the correct placement, in our build files, for the -fno-stack-protector flag.  Right now we just have http://hg.mozilla.org/tracemonkey/rev/663a3afb238e.
Comment 8 Ted Mielczarek [:ted.mielczarek] 2011-04-12 05:31:51 PDT
Ah. In general I'd prefer if we set these things in configure, since that's where we select compile flags, but js/src/Makefile is chock-full of them for historical reasons, so it's not really any worse. I already have bug 464328 on file to clean that up.
Comment 9 Luke Wagner [:luke] 2011-04-12 09:30:43 PDT
Great, thanks!

Note You need to log in before you can comment on or make changes to this bug.