644250 - fix Apple gcc bug caused by -fstack-protector

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

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

[(not currently active) Ted Mielczarek]

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]]

Nomis101: not sure why you changed OS... wrong bug?

Comment 3

[Nomis101]

Sorry, I didn't changed OS by purpose, I only CCed me.

Comment 4

[Luke Wagner [:luke]]

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]]

Ted: should I fix anything or can I just wontfix this?

Comment 6

[(not currently active) Ted Mielczarek]

Sorry, I don't know enough about GCC internals.

Comment 7

[Luke Wagner [:luke]]

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

[(not currently active) Ted Mielczarek]

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]]

Great, thanks!