Bug 172768 – [JSC] WTFGetBacktrace can return numberOfFrames == 0 in some architectures

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Caio Lima]

I faced the crash when working into ARMv6 JSC build to Raspberry Pi. The problem is that "backtrace" properly configured, it's function can return 0 as well.

[Caio Lima]

Created attachment 311620 [details]
Patch

[Build Bot]

Comment on attachment 311620 [details]
Patch

Attachment 311620 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3849987

New failing tests:
fast/css/target-fragment-match.html

[Build Bot]

Created attachment 311633 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Darin Adler]

Comment on attachment 311620 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=311620&action=review

> Source/WTF/wtf/StackTrace.cpp:59
> +    if (!numberOfFrames)
> +        return nullptr;

Not sure why this is OK. The StackTrace object in the local variable "trace" will leak every time this is called.

[Caio Lima]

Comment on attachment 311620 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=311620&action=review

>> Source/WTF/wtf/StackTrace.cpp:59
>> +        return nullptr;
> 
> Not sure why this is OK. The StackTrace object in the local variable "trace" will leak every time this is called.

You are right here. It's not Ok due the leak.

[Caio Lima]

Created attachment 313461 [details]
Patch

[Darin Adler]

Comment on attachment 313461 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=313461&action=review

This change doesn’t make things worse, but this existing code does things wrong. It’s not correct to allocate an object with placement new in memory that was allocated with fastMalloc and then later delete that object with a call to delete. Given how it’s allocated, the correct way to destroy that object is to explicitly call the destructor and then use fastFree to release the memory. I’d like to see captureStackTrace use std::unique_ptr or some other smart pointer for the return value so callers aren’t likely to get that wrong.

> Source/WTF/wtf/StackTrace.cpp:62
> +        delete trace;

As described above, this is incorrect, although the existing calling code outside the function has the same problem. Correct code for this would be:

    trace->~StackTrace();
    fastFree(trace);

[Saam Barati]

Comment on attachment 313461 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=313461&action=review

>> Source/WTF/wtf/StackTrace.cpp:62
>> +        delete trace;
> 
> As described above, this is incorrect, although the existing calling code outside the function has the same problem. Correct code for this would be:
> 
>     trace->~StackTrace();
>     fastFree(trace);

What about making StackTrace WTF_MAKE_FAST_ALLOCATED and leaving in this as delete, and changing how we ‘new’ above?

[Saam Barati]

(In reply to Darin Adler from comment #7)
> Comment on attachment 313461 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=313461&action=review
> 
> This change doesn’t make things worse, but this existing code does things
> wrong. It’s not correct to allocate an object with placement new in memory
> that was allocated with fastMalloc and then later delete that object with a
> call to delete. Given how it’s allocated, the correct way to destroy that
> object is to explicitly call the destructor and then use fastFree to release
> the memory. I’d like to see captureStackTrace use std::unique_ptr or some
> other smart pointer for the return value so callers aren’t likely to get
> that wrong.
+1 

> 
> > Source/WTF/wtf/StackTrace.cpp:62
> > +        delete trace;
> 
> As described above, this is incorrect, although the existing calling code
> outside the function has the same problem. Correct code for this would be:
> 
>     trace->~StackTrace();
>     fastFree(trace);

[Darin Adler]

Comment on attachment 313461 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=313461&action=review

>>>> Source/WTF/wtf/StackTrace.cpp:62
>>>> +        delete trace;
>>> 
>>> As described above, this is incorrect, although the existing calling code outside the function has the same problem. Correct code for this would be:
>>> 
>>>     trace->~StackTrace();
>>>     fastFree(trace);
>> 
>> What about making StackTrace WTF_MAKE_FAST_ALLOCATED and leaving in this as delete, and changing how we ‘new’ above?
> 
> 

I’m not sure that would be straightforward. I believe this uses fastMalloc because it wants to allocate storage past the end of the StackTrace, not for performance reasons.

[Caio Lima]

(In reply to Darin Adler from comment #7)
> Comment on attachment 313461 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=313461&action=review
> 
> This change doesn’t make things worse, but this existing code does things
> wrong. It’s not correct to allocate an object with placement new in memory
> that was allocated with fastMalloc and then later delete that object with a
> call to delete. Given how it’s allocated, the correct way to destroy that
> object is to explicitly call the destructor and then use fastFree to release
> the memory. I’d like to see captureStackTrace use std::unique_ptr or some
> other smart pointer for the return value so callers aren’t likely to get
> that wrong.
> 
> > Source/WTF/wtf/StackTrace.cpp:62
> > +        delete trace;
> 
> As described above, this is incorrect, although the existing calling code
> outside the function has the same problem. Correct code for this would be:
> 
>     trace->~StackTrace();
>     fastFree(trace);

In that case should we open a new bug to fix that? In that case, I should place a FIXME and fix that in the right way.

[Saam Barati]

(In reply to Caio Lima from comment #11)
> (In reply to Darin Adler from comment #7)
> > Comment on attachment 313461 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=313461&action=review
> > 
> > This change doesn’t make things worse, but this existing code does things
> > wrong. It’s not correct to allocate an object with placement new in memory
> > that was allocated with fastMalloc and then later delete that object with a
> > call to delete. Given how it’s allocated, the correct way to destroy that
> > object is to explicitly call the destructor and then use fastFree to release
> > the memory. I’d like to see captureStackTrace use std::unique_ptr or some
> > other smart pointer for the return value so callers aren’t likely to get
> > that wrong.
> > 
> > > Source/WTF/wtf/StackTrace.cpp:62
> > > +        delete trace;
> > 
> > As described above, this is incorrect, although the existing calling code
> > outside the function has the same problem. Correct code for this would be:
> > 
> >     trace->~StackTrace();
> >     fastFree(trace);
> 
> In that case should we open a new bug to fix that? In that case, I should
> place a FIXME and fix that in the right way.
I would just make the code do the right thing since the change seems small.

[Yusuke Suzuki]

Comment on attachment 313461 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=313461&action=review

>>>>>> Source/WTF/wtf/StackTrace.cpp:62
>>>>>> +        delete trace;
>>>>> 
>>>>> As described above, this is incorrect, although the existing calling code outside the function has the same problem. Correct code for this would be:
>>>>> 
>>>>>     trace->~StackTrace();
>>>>>     fastFree(trace);
>>>> 
>>>> What about making StackTrace WTF_MAKE_FAST_ALLOCATED and leaving in this as delete, and changing how we ‘new’ above?
>>> 
>>> 
>> 
>> I’m not sure that would be straightforward. I believe this uses fastMalloc because it wants to allocate storage past the end of the StackTrace, not for performance reasons.
> 
> In that case should we open a new bug to fix that? In that case, I should place a FIXME and fix that in the right way.

I think we should annotate it with WTF_MAKE_FAST_ALLOCATED. The situation is the same to StringImpl, which may has trailing string content too.
By annotating WTF_MAKE_FAST_ALLOCATED, `delete trace` becomes completely valid.
And I think we should add `std::unique_ptr<StackTrace> create(int maxFrames)` private static function and use it in captureStackTrace.
By doing so, we can avoid explicit `delete`.

And I also think that `captureStackTrace` should return `std::unique_ptr<StackTrace>`.

[Caio Lima]

Created attachment 314215 [details]
Patch

[Keith Miller]

Comment on attachment 314215 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=314215&action=review

r=me with some changes.

> Source/WTF/ChangeLog:8
> +        In some architectures, like ARMv6 running into Raspberry pi, the

typo: into -> on a

> Source/WTF/ChangeLog:11
> +        runtime crash.

typo: a runtime crash

> Source/WTF/ChangeLog:13
> +        avoid runtime crash in such case.

typo: ditto "a runtime crash"

> Source/JavaScriptCore/runtime/ExceptionScope.cpp:60
>      currentStack->dump(out, "    ");

I think this should be:

if (currentStack)
    currentStack->dump(out, "    ");

given that the captureStackTrace function can return a nullptr.

I think you also need a null check on the m_vm.nativeStackTraceOfLastThrow()->dump(out, " "); below.

[Mark Lam]

Comment on attachment 314215 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=314215&action=review

ExceptionScope::unexpectedExceptionMessage() expects the StackTrace object to always succeed, which I think is a reasonable expectation.  There's nothing that says that the StackTrace object has to contain any frames.  How about just doing a more minimal change instead (see comment in StackTrace::captureStackTrace() below) ...

> Source/WTF/wtf/StackTrace.cpp:72
> +    if (!numberOfFrames)
> +        return nullptr;
> +
>      RELEASE_ASSERT(numberOfFrames >= framesToSkip);
>      trace->m_size = numberOfFrames - framesToSkip;

I suggest undoing everything else, and changing this to:

    if (numberOfFrames) {
        RELEASE_ASSERT(numberOfFrames >= framesToSkip);
        trace->m_size = numberOfFrames - framesToSkip;
    } else
        trace->m_size = 0;

[Caio Lima]

Created attachment 314261 [details]
Patch

[Caio Lima]

(In reply to Mark Lam from comment #16)
> Comment on attachment 314215 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=314215&action=review
> 
> ExceptionScope::unexpectedExceptionMessage() expects the StackTrace object
> to always succeed, which I think is a reasonable expectation.  There's
> nothing that says that the StackTrace object has to contain any frames.  How
> about just doing a more minimal change instead (see comment in
> StackTrace::captureStackTrace() below) ...
> 
> > Source/WTF/wtf/StackTrace.cpp:72
> > +    if (!numberOfFrames)
> > +        return nullptr;
> > +
> >      RELEASE_ASSERT(numberOfFrames >= framesToSkip);
> >      trace->m_size = numberOfFrames - framesToSkip;
> 
> I suggest undoing everything else, and changing this to:
> 
>     if (numberOfFrames) {
>         RELEASE_ASSERT(numberOfFrames >= framesToSkip);
>         trace->m_size = numberOfFrames - framesToSkip;
>     } else
>         trace->m_size = 0;

Done.

[Mark Lam]

Comment on attachment 314261 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=314261&action=review

> Source/WTF/wtf/StackTrace.cpp:64
> +        trace->m_capacity = maxFrames;

This is still incorrect.  capacity and size are 2 different concepts.  trace->m_capacity needs to be set even if trace->m_size is 0.  If you look in StackTrace.hpp, you'll see that trace->m_capacity being set has a special meaning: it means that we did allocate memory for the StackTrace trace (which we did because we ensure that maxFrames = std::max(1, maxFrames) above).  In practice, though not setting trace->m_capacity here means that stack() will return a random address, size() being 0 should stop the client from using the buffer.  Regardless, let's do this properly and set trace->m_capacity so that we tell the truth about how we allocate the buffer.

[Caio Lima]

Created attachment 314334 [details]
Patch

[Caio Lima]

(In reply to Mark Lam from comment #19)
> Comment on attachment 314261 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=314261&action=review
> 
> > Source/WTF/wtf/StackTrace.cpp:64
> > +        trace->m_capacity = maxFrames;
> 
> This is still incorrect.  capacity and size are 2 different concepts. 
> trace->m_capacity needs to be set even if trace->m_size is 0.  If you look
> in StackTrace.hpp, you'll see that trace->m_capacity being set has a special
> meaning: it means that we did allocate memory for the StackTrace trace
> (which we did because we ensure that maxFrames = std::max(1, maxFrames)
> above).  In practice, though not setting trace->m_capacity here means that
> stack() will return a random address, size() being 0 should stop the client
> from using the buffer.  Regardless, let's do this properly and set
> trace->m_capacity so that we tell the truth about how we allocate the buffer.

Makes sense. I misread your suggestion.

BTW, don't you think that returning raw pointer here is a potential source of memory leak?

[Mark Lam]

Comment on attachment 314334 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=314334&action=review

r=me with ChangeLog fix.

> Source/WTF/ChangeLog:12
> +        This Patch is adding a guard for the case described above to

typo: /Patch/patch/.

[Mark Lam]

(In reply to Caio Lima from comment #21)
> BTW, don't you think that returning raw pointer here is a potential source
> of memory leak?

Yes, it is true that there's theoretical risk.  However, I recall opting to go this route because there was a reason that makes returning a direct pointer the better solution here.  I may have remembered wrongly, or perhaps, the old reason is no longer valid. We may want to revisit this at a later point in time (it's not critical for the present, and not for this patch).  If you wish to investigate it carefully and make sure that there are no longer any need that requires returning a direct pointer, then feel free to create a new bug and propose a solution.

[Caio Lima]

Created attachment 314385 [details]
Patch for landing

Fixed typos

[Caio Lima]

Created attachment 314386 [details]
Patch for landing

Rebased with master

[Caio Lima]

(In reply to Mark Lam from comment #23)
> (In reply to Caio Lima from comment #21)
> > BTW, don't you think that returning raw pointer here is a potential source
> > of memory leak?
> 
> Yes, it is true that there's theoretical risk.  However, I recall opting to
> go this route because there was a reason that makes returning a direct
> pointer the better solution here.  I may have remembered wrongly, or
> perhaps, the old reason is no longer valid. We may want to revisit this at a
> later point in time (it's not critical for the present, and not for this
> patch).  If you wish to investigate it carefully and make sure that there
> are no longer any need that requires returning a direct pointer, then feel
> free to create a new bug and propose a solution.

Ok.

[WebKit Commit Bot]

Comment on attachment 314386 [details]
Patch for landing

Clearing flags on attachment: 314386

Committed r219053: <http://trac.webkit.org/changeset/219053>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Guillaume Emont]

On a MIPS system where libunwind has not been compiled, backtrace() seems to always return 1, and I still hit the ASSERT(). Should the test be on numberOfFrames>1 ? Or should the ASSERT() just be an if and not an ASSERT()? I'm not sure I understand the spirit behind that ASSERT.

[Mark Lam]

(In reply to Guillaume Emont from comment #29)
> On a MIPS system where libunwind has not been compiled, backtrace() seems to
> always return 1, and I still hit the ASSERT(). Should the test be on
> numberOfFrames>1 ? Or should the ASSERT() just be an if and not an ASSERT()?
> I'm not sure I understand the spirit behind that ASSERT.

Your bug here is not with the StackTrace implementation.  It is that Platform.h is lying about MIPS having a backtrace implementation.  To fix this: change Platform.h to not define HAVE_BACKTRACE for CPU_MIPS.

[Guillaume Emont]

(In reply to Mark Lam from comment #30)
> (In reply to Guillaume Emont from comment #29)
> > On a MIPS system where libunwind has not been compiled, backtrace() seems to
> > always return 1, and I still hit the ASSERT(). Should the test be on
> > numberOfFrames>1 ? Or should the ASSERT() just be an if and not an ASSERT()?
> > I'm not sure I understand the spirit behind that ASSERT.
> 
> Your bug here is not with the StackTrace implementation.  It is that
> Platform.h is lying about MIPS having a backtrace implementation.  To fix
> this: change Platform.h to not define HAVE_BACKTRACE for CPU_MIPS.

I have not tried yet, as I think I need to recompile a whole system for that, but my understanding is that I get a 1 return value because I do not have libunwind, but that if I had libunwind available when building gcc/libgcc, then I would get a properly working backtrace(). In other words: I do think there is support for backtrace() on MIPS, just not on my specific system that lacks libunwind. (All of that pending verification).

[Mark Lam]

(In reply to Guillaume Emont from comment #31)
> (In reply to Mark Lam from comment #30)
> > (In reply to Guillaume Emont from comment #29)
> > > On a MIPS system where libunwind has not been compiled, backtrace() seems to
> > > always return 1, and I still hit the ASSERT(). Should the test be on
> > > numberOfFrames>1 ? Or should the ASSERT() just be an if and not an ASSERT()?
> > > I'm not sure I understand the spirit behind that ASSERT.
> > 
> > Your bug here is not with the StackTrace implementation.  It is that
> > Platform.h is lying about MIPS having a backtrace implementation.  To fix
> > this: change Platform.h to not define HAVE_BACKTRACE for CPU_MIPS.
> 
> I have not tried yet, as I think I need to recompile a whole system for
> that, but my understanding is that I get a 1 return value because I do not
> have libunwind, but that if I had libunwind available when building
> gcc/libgcc, then I would get a properly working backtrace(). In other words:
> I do think there is support for backtrace() on MIPS, just not on my specific
> system that lacks libunwind. (All of that pending verification).

Can you file a separate bug to track this for MIPS if needed?  Better to hold the MIPS specific discussion there.