Bug 186980 – Asan false positive: stack use after scope under WebCore::ApplyPropertyBorderImageModifier in WebCore::Length::Length(WebCore::Length&&)

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Michael Catanzaro]

Running an asan build of WebKit, I see this error when loading Epiphany's about:overview page (the default page that loads on startup, hopefully I can find a different reproducer). The bug does not seem straightforward; I've been staring at it for quite a while without figuring out the problem.

Note that the memcpy in the Length::Length(Length&& other) constructor (and the other constructors, and the assignment operators) is at best highly questionable, since Length is not a trivially copyable type. I hope that's not causing problems here.

Note also that the other.m_type = Auto is a bit confusing, but it's there to prevent the destructor of the temporary Length from running deref().


==29135==ERROR: AddressSanitizer: stack-use-after-scope on address 0x7ffd55a7a780 at pc 0x7fe6e4eedaf7 bp 0x7ffd55a7a530 sp 0x7ffd55a7a520
WRITE of size 8 at 0x7ffd55a7a780 thread T0
    #0 0x7fe6e4eedaf6 in WebCore::Length::Length(WebCore::Length&&) /home/mcatanzaro/Projects/WebKit/Source/WebCore/platform/Length.h:185
    #1 0x7fe6e4f80be5 in WebCore::RectEdges<WebCore::Length>::RectEdges<WebCore::Length>(WebCore::Length&&, WebCore::Length&&, WebCore::Length&&, WebCore::Length&&) /home/mcatanzaro/Projects/WebKit/Source/WebCore/platform/RectEdges.h:39
    #2 0x7fe6e4eeea79 in WebCore::LengthBox::LengthBox(WebCore::Length&&, WebCore::Length&&, WebCore::Length&&, WebCore::Length&&) /home/mcatanzaro/Projects/WebKit/Source/WebCore/platform/LengthBox.h:53
    #3 0x7fe6e4f97ca9 in WebCore::ApplyPropertyBorderImageModifier<(WebCore::BorderImageType)0, (WebCore::BorderImageModifierType)3>::applyInitialValue(WebCore::StyleResolver&) (/home/mcatanzaro/Projects/GNOME/install/lib/libwebkit2gtk-4.0.so.37+0x5bccca9)

(Splitting the trace here because the interesting parts are above this line.)

    #4 0x7fe6e4f33ff9 in WebCore::StyleBuilderCustom::applyInitialBorderImageWidth(WebCore::StyleResolver&) (/home/mcatanzaro/Projects/GNOME/install/lib/libwebkit2gtk-4.0.so.37+0x5b68ff9)
    #5 0x7fe6e4edf735 in WebCore::StyleBuilder::applyProperty(WebCore::CSSPropertyID, WebCore::StyleResolver&, WebCore::CSSValue&, bool, bool) DerivedSources/WebCore/StyleBuilder.cpp:5778
    #6 0x7fe6e5d9e8a8 in WebCore::StyleResolver::applyProperty(WebCore::CSSPropertyID, WebCore::CSSValue*, WebCore::SelectorChecker::LinkMatchMask, WebCore::StyleResolver::MatchResult const*) /home/mcatanzaro/Projects/WebKit/Source/WebCore/css/StyleResolver.cpp:1705
    #7 0x7fe6e5da2f2c in WebCore::StyleResolver::CascadedProperties::Property::apply(WebCore::StyleResolver&, WebCore::StyleResolver::MatchResult const*) /home/mcatanzaro/Projects/WebKit/Source/WebCore/css/StyleResolver.cpp:2242
    #8 0x7fe6e5da2d8e in WebCore::StyleResolver::CascadedProperties::applyDeferredProperties(WebCore::StyleResolver&, WebCore::StyleResolver::MatchResult const*) /home/mcatanzaro/Projects/WebKit/Source/WebCore/css/StyleResolver.cpp:2230
    #9 0x7fe6e5d9cc29 in WebCore::StyleResolver::applyMatchedProperties(WebCore::StyleResolver::MatchResult const&, WebCore::Element const&, WebCore::StyleResolver::ShouldUseMatchedPropertiesCache) /home/mcatanzaro/Projects/WebKit/Source/WebCore/css/StyleResolver.cpp:1423
    #10 0x7fe6e5d9408d in WebCore::StyleResolver::styleForElement(WebCore::Element const&, WebCore::RenderStyle const*, WebCore::RenderStyle const*, WebCore::RuleMatchingBehavior, WebCore::SelectorFilter const*) /home/mcatanzaro/Projects/WebKit/Source/WebCore/css/StyleResolver.cpp:396
    #11 0x7fe6e83dc95f in WebCore::Style::TreeResolver::styleForElement(WebCore::Element&, WebCore::RenderStyle const&) /home/mcatanzaro/Projects/WebKit/Source/WebCore/style/StyleTreeResolver.cpp:131
    #12 0x7fe6e83dd379 in WebCore::Style::TreeResolver::resolveElement(WebCore::Element&) /home/mcatanzaro/Projects/WebKit/Source/WebCore/style/StyleTreeResolver.cpp:202
    #13 0x7fe6e83e041b in WebCore::Style::TreeResolver::resolveComposedTree() /home/mcatanzaro/Projects/WebKit/Source/WebCore/style/StyleTreeResolver.cpp:504
    #14 0x7fe6e83e0df7 in WebCore::Style::TreeResolver::resolve() /home/mcatanzaro/Projects/WebKit/Source/WebCore/style/StyleTreeResolver.cpp:564
    #15 0x7fe6e600f03b in WebCore::Document::resolveStyle(WebCore::Document::ResolveStyleType) /home/mcatanzaro/Projects/WebKit/Source/WebCore/dom/Document.cpp:1849
    #16 0x7fe6e601010b in WebCore::Document::updateStyleIfNeeded() /home/mcatanzaro/Projects/WebKit/Source/WebCore/dom/Document.cpp:1967
    #17 0x7fe6e6031892 in WebCore::Document::finishedParsing() /home/mcatanzaro/Projects/WebKit/Source/WebCore/dom/Document.cpp:5436
    #18 0x7fe6e69b24d5 in WebCore::HTMLConstructionSite::finishedParsing() /home/mcatanzaro/Projects/WebKit/Source/WebCore/html/parser/HTMLConstructionSite.cpp:419
    #19 0x7fe6e6a200fb in WebCore::HTMLTreeBuilder::finished() /home/mcatanzaro/Projects/WebKit/Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2841
    #20 0x7fe6e69bafdb in WebCore::HTMLDocumentParser::end() /home/mcatanzaro/Projects/WebKit/Source/WebCore/html/parser/HTMLDocumentParser.cpp:406
    #21 0x7fe6e69bb0b3 in WebCore::HTMLDocumentParser::attemptToRunDeferredScriptsAndEnd() /home/mcatanzaro/Projects/WebKit/Source/WebCore/html/parser/HTMLDocumentParser.cpp:415
    #22 0x7fe6e69b8d37 in WebCore::HTMLDocumentParser::prepareToStopParsing() /home/mcatanzaro/Projects/WebKit/Source/WebCore/html/parser/HTMLDocumentParser.cpp:138
    #23 0x7fe6e69bb123 in WebCore::HTMLDocumentParser::attemptToEnd() /home/mcatanzaro/Projects/WebKit/Source/WebCore/html/parser/HTMLDocumentParser.cpp:427
    #24 0x7fe6e69bb244 in WebCore::HTMLDocumentParser::finish() /home/mcatanzaro/Projects/WebKit/Source/WebCore/html/parser/HTMLDocumentParser.cpp:455
    #25 0x7fe6e6dabee3 in WebCore::DocumentWriter::end() /home/mcatanzaro/Projects/WebKit/Source/WebCore/loader/DocumentWriter.cpp:284
    #26 0x7fe6e6d92448 in WebCore::DocumentLoader::finishedLoading() /home/mcatanzaro/Projects/WebKit/Source/WebCore/loader/DocumentLoader.cpp:433
    #27 0x7fe6e6d97612 in WebCore::DocumentLoader::continueAfterContentPolicy(WebCore::PolicyAction) /home/mcatanzaro/Projects/WebKit/Source/WebCore/loader/DocumentLoader.cpp:948
    #28 0x7fe6e6d96173 in WebCore::DocumentLoader::responseReceived(WebCore::ResourceResponse const&, WTF::CompletionHandler<void ()>&&) /home/mcatanzaro/Projects/WebKit/Source/WebCore/loader/DocumentLoader.cpp:820
    #29 0x7fe6e6d929c1 in WebCore::DocumentLoader::handleSubstituteDataLoadNow() /home/mcatanzaro/Projects/WebKit/Source/WebCore/loader/DocumentLoader.cpp:468
    #30 0x7fe6e6ddfa74 in void std::__invoke_impl<void, void (WebCore::DocumentLoader::*&)(), WebCore::DocumentLoader*&>(std::__invoke_memfun_deref, void (WebCore::DocumentLoader::*&)(), WebCore::DocumentLoader*&) (/home/mcatanzaro/Projects/GNOME/install/lib/libwebkit2gtk-4.0.so.37+0x7a14a74)
    #31 0x7fe6e6ddf924 in std::__invoke_result<void (WebCore::DocumentLoader::*&)(), WebCore::DocumentLoader*&>::type std::__invoke<void (WebCore::DocumentLoader::*&)(), WebCore::DocumentLoader*&>(void (WebCore::DocumentLoader::*&)(), WebCore::DocumentLoader*&) (/home/mcatanzaro/Projects/GNOME/install/lib/libwebkit2gtk-4.0.so.37+0x7a14924)
    #32 0x7fe6e6ddf851 in void std::_Bind<void (WebCore::DocumentLoader::*(WebCore::DocumentLoader*))()>::__call<void, , 0ul>(std::tuple<>&&, std::_Index_tuple<0ul>) /usr/include/c++/8/functional:400
    #33 0x7fe6e6ddf72e in void std::_Bind<void (WebCore::DocumentLoader::*(WebCore::DocumentLoader*))()>::operator()<, void>() /usr/include/c++/8/functional:484
    #34 0x7fe6e6ddf687 in WTF::Function<void ()>::CallableWrapper<std::_Bind<void (WebCore::DocumentLoader::*(WebCore::DocumentLoader*))()> >::call() (/home/mcatanzaro/Projects/GNOME/install/lib/libwebkit2gtk-4.0.so.37+0x7a14687)
    #35 0x7fe6e2332729 in WTF::Function<void ()>::operator()() const /home/mcatanzaro/Projects/WebKit/Source/WTF/wtf/Function.h:56
    #36 0x7fe6e2353977 in WebCore::Timer::fired() /home/mcatanzaro/Projects/WebKit/Source/WebCore/platform/Timer.h:131
    #37 0x7fe6e747412e in WebCore::ThreadTimers::sharedTimerFiredInternal() /home/mcatanzaro/Projects/WebKit/Source/WebCore/platform/ThreadTimers.cpp:117
    #38 0x7fe6e7472ecc in operator() /home/mcatanzaro/Projects/WebKit/Source/WebCore/platform/ThreadTimers.cpp:69
    #39 0x7fe6e748f603 in call DerivedSources/ForwardingHeaders/wtf/Function.h:101
    #40 0x7fe6e2332729 in WTF::Function<void ()>::operator()() const /home/mcatanzaro/Projects/WebKit/Source/WTF/wtf/Function.h:56
    #41 0x7fe6e741ff05 in WebCore::MainThreadSharedTimer::fired() /home/mcatanzaro/Projects/WebKit/Source/WebCore/platform/MainThreadSharedTimer.cpp:54
    #42 0x7fe6e7428d60 in WTF::RunLoop::Timer<WebCore::MainThreadSharedTimer>::fired() DerivedSources/ForwardingHeaders/wtf/RunLoop.h:148
    #43 0x7fe6dc548ff7 in operator() /home/mcatanzaro/Projects/WebKit/Source/WTF/wtf/glib/RunLoopGLib.cpp:170
    #44 0x7fe6dc549082 in _FUN /home/mcatanzaro/Projects/WebKit/Source/WTF/wtf/glib/RunLoopGLib.cpp:176
    #45 0x7fe6dc547f95 in operator() /home/mcatanzaro/Projects/WebKit/Source/WTF/wtf/glib/RunLoopGLib.cpp:45
    #46 0x7fe6dc547fc5 in _FUN /home/mcatanzaro/Projects/WebKit/Source/WTF/wtf/glib/RunLoopGLib.cpp:46
    #47 0x7fe6d04343ad in g_main_dispatch ../../../../Projects/glib/glib/gmain.c:3177
    #48 0x7fe6d043522f in g_main_context_dispatch ../../../../Projects/glib/glib/gmain.c:3830
    #49 0x7fe6d0435413 in g_main_context_iterate ../../../../Projects/glib/glib/gmain.c:3903
    #50 0x7fe6d0435839 in g_main_loop_run ../../../../Projects/glib/glib/gmain.c:4099
    #51 0x7fe6dc54883f in WTF::RunLoop::run() /home/mcatanzaro/Projects/WebKit/Source/WTF/wtf/glib/RunLoopGLib.cpp:96
    #52 0x7fe6e3af59d0 in int WebKit::ChildProcessMain<WebKit::WebProcess, WebKit::WebProcessMain>(int, char**) /home/mcatanzaro/Projects/WebKit/Source/WebKit/Shared/unix/ChildProcessMain.h:61
    #53 0x7fe6e3af548c in WebProcessMainUnix /home/mcatanzaro/Projects/WebKit/Source/WebKit/WebProcess/gtk/WebProcessMainGtk.cpp:67
    #54 0x400d90 in main /home/mcatanzaro/Projects/WebKit/Source/WebKit/WebProcess/EntryPoint/unix/WebProcessMain.cpp:52
    #55 0x7fe6cd8eb18a in __libc_start_main ../csu/libc-start.c:308
    #56 0x400c59 in _start (/home/mcatanzaro/Projects/GNOME/install/libexec/webkit2gtk-4.0/WebKitWebProcess+0x400c59)

Address 0x7ffd55a7a780 is located in stack of thread T0 at offset 352 in frame
    #0 0x7fe6e4f97af0 in WebCore::ApplyPropertyBorderImageModifier<(WebCore::BorderImageType)0, (WebCore::BorderImageModifierType)3>::applyInitialValue(WebCore::StyleResolver&) (/home/mcatanzaro/Projects/GNOME/install/lib/libwebkit2gtk-4.0.so.37+0x5bccaf0)

  This frame has 6 object(s):
    [32, 40) 'image'
    [96, 104) '<unknown>'
    [160, 168) '<unknown>'
    [224, 232) '<unknown>'
    [288, 296) '<unknown>'
    [352, 384) '<unknown>' <== Memory access at offset 352 is inside this variable
HINT: this may be a false positive if your program uses some custom stack unwind mechanism or swapcontext
      (longjmp and C++ exceptions *are* supported)

[Radar WebKit Bug Importer]

<rdar://problem/41409838>

[Michael Catanzaro]

BTW, a workaround is to just remove the Length(Length&&) constructor. Since it's implemented with a copy anyway, it's not any more efficient than the copy constructor, so that might even be a reasonable solution.

[Brent Fulgham]

Could you add the "about:overview" page contents that generated this crash? I'd like to confirm we don't have a similar issue in iOS/macOS.

[Michael Catanzaro]

(In reply to Brent Fulgham from comment #3)
> Could you add the "about:overview" page contents that generated this crash?
> I'd like to confirm we don't have a similar issue in iOS/macOS.

I'm afraid it depends on local files and Epiphany-specific JavaScript, but no worries: I can reproduce the same crash on both apple.com and google.com (simpler). Just loading either page with asan enabled should suffice.

There's no platform-specific code anywhere in the trace (which ends at the llint boundary) so it's very likely a cross-platform problem.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #2)
> BTW, a workaround is to just remove the Length(Length&&) constructor. Since
> it's implemented with a copy anyway, it's not any more efficient than the
> copy constructor, so that might even be a reasonable solution.

Still, I wish I understood what's going wrong.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #2)
> BTW, a workaround is to just remove the Length(Length&&) constructor. Since
> it's implemented with a copy anyway, it's not any more efficient than the
> copy constructor, so that might even be a reasonable solution.

Actually that's a lie, that just shifts the error to the copy constructor. I must have messed up somehow when I tried that yesterday.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #0)
> Note that the memcpy in the Length::Length(Length&& other) constructor (and
> the other constructors, and the assignment operators) is at best highly
> questionable, since Length is not a trivially copyable type. I hope that's
> not causing problems here.

The error still occurs when replacing the memcpy with manual assignments, so that's not it.

[Michael Catanzaro]

I found in the GCC 7 release notes, which added -fsanitize-address-use-after-scope:

"""
Compared to the LLVM compiler, where the option already exists, the implementation in the GCC compiler has some improvements and advantages:

[...]

C++ temporaries are sanitized.
"""

So if Clang's asan is unable to detect this issue, that's probably why.

[Michael Catanzaro]

I'm really having trouble with this one.

Here is some debug:

Length::Length(Length&& other)
{
WTFLogAlways("%s: other=%p", __PRETTY_FUNCTION__, &other);

WTFLogAlways("other.m_hasQuirk=%d", other.m_hasQuirk);
WTFLogAlways("other.m_type=%d", other.m_type);
WTFLogAlways("other.m_isFloat=%d", other.m_isFloat);

WTFLogAlways("this=%p", this); <-- Last statement printed
WTFLogAlways("m_hasQuirk=%d", m_hasQuirk); <-- Crash occurs here
WTFLogAlways("m_type=%d", m_type);
WTFLogAlways("m_isFloat=%d", m_isFloat);

    memcpy(this, &other, sizeof(Length));
    other.m_type = Auto;
}

It seems that using the values of other is perfectly fine, but not the data members of this, even though they should have just been initialized by the constructor, which is crazy.

[Michael Catanzaro]

*** Bug 187122 has been marked as a duplicate of this bug. ***

[Darin Adler]

(In reply to Michael Catanzaro from comment #9)
> Length::Length(Length&& other)
> {
> WTFLogAlways("%s: other=%p", __PRETTY_FUNCTION__, &other);
> 
> WTFLogAlways("other.m_hasQuirk=%d", other.m_hasQuirk);
> WTFLogAlways("other.m_type=%d", other.m_type);
> WTFLogAlways("other.m_isFloat=%d", other.m_isFloat);
> 
> WTFLogAlways("this=%p", this); <-- Last statement printed
> WTFLogAlways("m_hasQuirk=%d", m_hasQuirk); <-- Crash occurs here
> WTFLogAlways("m_type=%d", m_type);
> WTFLogAlways("m_isFloat=%d", m_isFloat);
> 
>     memcpy(this, &other, sizeof(Length));
>     other.m_type = Auto;
> }
> 
> It seems that using the values of other is perfectly fine, but not the data
> members of this, even though they should have just been initialized by the
> constructor, which is crazy.

It’s not correct to say that these data members "should have just been initialized by the constructor". The function here *is* the constructor and at the point of the log statement m_hasQuirk has *not* been initialized yet.

It’s correct for ASan to object to *reading* this->m_hasQuirk before any code has *written* to it.

I don’t know how this logging relates to the bug, though.

[Darin Adler]

Just a guess, but I think the issue might possibly be that ASan doesn’t understand the union in Length.

This Length move constructor needs to copy m_intValue, but m_floatValue is not initialized. The code, however, tries to move the entire object. ASan gets upset that we are moving some uninitialized bugs from one object to another, and doesn’t know that those uninitialized bytes won’t be looked at because of m_isFloat.

Here’s a way to test this theory. We can write the following version of the move constructor:

    if (other.m_isFloat)
        m_floatValue = other.m_floatValue;
    else
        m_intValue = other.m_intValue;
    m_hasQuirk = other.m_hasQuirk;
    m_type = other.m_type;
    m_isFloat = other.m_isFloat;
    other.m_type = Auto;

If this version makes the ASan complaints go away, then it’s a false positive and we can think about how to make ASan happy instead of worrying that it’s detecting a bug.

[Michael Catanzaro]

(In reply to Darin Adler from comment #11)
> It’s not correct to say that these data members "should have just been
> initialized by the constructor". The function here *is* the constructor and
> at the point of the log statement m_hasQuirk has *not* been initialized yet.

But we are past the initializer list at this point, right? So everything should be initialized to default values already?

I think it will still crash if I replace the read with a write to any member of this. I tested this yesterday; I'll do so again now to be completely sure.

(In reply to Darin Adler from comment #12)
> Here’s a way to test this theory. We can write the following version of the
> move constructor:
> 
>     if (other.m_isFloat)
>         m_floatValue = other.m_floatValue;
>     else
>         m_intValue = other.m_intValue;
>     m_hasQuirk = other.m_hasQuirk;
>     m_type = other.m_type;
>     m_isFloat = other.m_isFloat;
>     other.m_type = Auto;

I tried almost exactly this yesterday (the only difference being I also tested m_type and copied m_calculationValueHandle instead if it was Calculated, but this problem does not involved calculated Lengths, so that should not matter). Unfortunately, that did not placate asan. It crashed as soon as I tried assigning to the members of this.

That doesn't mean you are wrong to suspect the union; it could still be involved somehow, perhaps.

[Michael Catanzaro]

I'll test this now, just to be certain that I didn't screw up or misremember my testing from yesterday:

Length::Length(Length&& other)
{
     if (other.isCalculated())
         m_calculationValueHandle = other.m_calculationValueHandle;
     else if (other.m_isFloat)
         m_floatValue = other.m_floatValue;
     else
         m_intValue = other.m_intValue; // <-- pretty sure it will crash here

     m_hasQuirk = other.m_hasQuirk;
     m_type = other.m_type;
     m_isFloat = other.m_isFloat;
     other.m_type = Auto;
}

[Michael Catanzaro]

(In reply to Darin Adler from comment #11)
> It’s not correct to say that these data members "should have just been
> initialized by the constructor". The function here *is* the constructor and
> at the point of the log statement m_hasQuirk has *not* been initialized yet.
> 
> It’s correct for ASan to object to *reading* this->m_hasQuirk before any
> code has *written* to it.
> 
> I don’t know how this logging relates to the bug, though.

Alicia (CCed) has convinced me that you're right about this, because C++ does not initialize primitive types to default values.

[Darin Adler]

Exactly.

Default initialization <https://en.cppreference.com/w/cpp/language/default_initialization> is done before this point, but a careful reading will show that such initialization doesn’t initialize all data members. The terminology used is "initialized to indeterminate values", in other words, not initialized.

[Darin Adler]

(In reply to Darin Adler from comment #12)
> uninitialized bugs

Heh, "uninitialized bugs", "uninitialized bytes", whichever you prefer.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #14)
> I'll test this now, just to be certain that I didn't screw up or misremember
> my testing from yesterday:
> 
> Length::Length(Length&& other)
> {
>      if (other.isCalculated())
>          m_calculationValueHandle = other.m_calculationValueHandle;
>      else if (other.m_isFloat)
>          m_floatValue = other.m_floatValue;
>      else
>          m_intValue = other.m_intValue; // <-- pretty sure it will crash here
> 
>      m_hasQuirk = other.m_hasQuirk;
>      m_type = other.m_type;
>      m_isFloat = other.m_isFloat;
>      other.m_type = Auto;
> }

The result is as expected, the stack-use-after-free crash moves to the line where m_intValue is initialized. Super weird.

[Alicia Boya García]

Created attachment 343849 [details]
move-code-outside.patch

This bug is super weird. These kind of errors usually derive from taking the address of a defunt temporary but I see no obvious instance of that.

Look at this patch, it makes the error go away just by moving the conflictive code to a separate function, but does exactly the same: same allocations, same copies, same method calls... I'm starting to wonder if the compiler may be calculating the wrong frame size for applyInitialValue() or something of the likes.

gcc (GCC) 8.1.1 20180502 (Red Hat 8.1.1-1)

[Darin Adler]

If the error is happening when reading from an address, we should figure out exactly which address it is. I would expect it to be a stack location from inside the call of applyInitialValue. Is the stack value consistent with this assumption? We have the stack frame addresses of the various functions in the backtrace to check against.

[Darin Adler]

Can we make this failure happen inside a debugger? I think it’s really relevant to poke around and see exactly what memory we are talking about.

[Darin Adler]

Some more ideas for debugging: We need to verify that it’s the *read* of other.m_intValue that is the issue. If optimization is turned off then I think we should rearrange this move constructor so that other.m_hasQuirk, other.m_type, and other.m_isFloat are all read first to verify that it’s specifically other.m_intValue that it’s complaining about, not the entire "other" object. Then drill into why other.m_intValue is supposed to be initialized and not invalidated.

[Michael Catanzaro]

I'm going to fire up gdb in a second. First, a couple more notes. Alicia discovered what we believe to be a subtle bug in the RectEdges constructor:

--- a/Source/WebCore/platform/RectEdges.h
+++ b/Source/WebCore/platform/RectEdges.h
@@ -36,7 +36,7 @@ public:
     
     template<typename U>
     RectEdges(U&& top, U&& right, U&& bottom, U&& left)
-        : m_sides({ { std::forward<T>(top), std::forward<T>(right), std::forward<T>(bottom), std::forward<T>(left) } })
+        : m_sides({ { std::forward<U>(top), std::forward<U>(right), std::forward<U>(bottom), std::forward<U>(left) } })
     { }
     
     T& at(PhysicalBoxSide side) { return m_sides[static_cast<size_t>(side)]; }

(The template type is wrong.)

And then I found some misplaced WTFMoves (moving from copies rather than rvalue references).

diff --git a/Source/WebCore/rendering/style/NinePieceImage.h b/Source/WebCore/rendering/style/NinePieceIm
age.h
index d5465e94e92..ecef25095f8 100644
--- a/Source/WebCore/rendering/style/NinePieceImage.h
+++ b/Source/WebCore/rendering/style/NinePieceImage.h
@@ -111,16 +111,16 @@ public:
     void setImage(RefPtr<StyleImage>&& image) { m_data.access().image = WTFMove(image); }
 
     const LengthBox& imageSlices() const { return m_data->imageSlices; }
-    void setImageSlices(LengthBox slices) { m_data.access().imageSlices = WTFMove(slices); }
+    void setImageSlices(LengthBox slices) { m_data.access().imageSlices = slices; }
 
     bool fill() const { return m_data->fill; }
     void setFill(bool fill) { m_data.access().fill = fill; }
 
     const LengthBox& borderSlices() const { return m_data->borderSlices; }
-    void setBorderSlices(LengthBox slices) { m_data.access().borderSlices = WTFMove(slices); }
+    void setBorderSlices(LengthBox slices) { m_data.access().borderSlices = slices; }
 
     const LengthBox& outset() const { return m_data->outset; }
-    void setOutset(LengthBox outset) { m_data.access().outset = WTFMove(outset); }
+    void setOutset(LengthBox outset) { m_data.access().outset = outset; }
 
     ENinePieceImageRule horizontalRule() const { return static_cast<ENinePieceImageRule>(m_data->horizontalRule); }
     void setHorizontalRule(ENinePieceImageRule rule) { m_data.access().horizontalRule = rule; }

[Zan Dobersek]

ASan-generated code in ApplyPropertyBorderImageModifier<>::applyInitialValue() allocates the shadow memory necessary to accompany the stack in this method. In this method, that's 52 bytes. This is how this memory looks at the start:

(gdb) x /13x $r12 + 0x7fff8000
0x10006c7aec02:	0x00000000	0x00000000	0x00000000	0x00000000
0x10006c7aec12:	0x00000000	0x00000000	0x00000000	0x00000000
0x10006c7aec22:	0x00000000	0x00000000	0x00000000	0xf8f8f8f8
0x10006c7aec32:	0x00000000

There's already the 0xf8f8f8f8 value there. I don't know how ASan exactly allocates this, but I'm pretty sure it expects that the memory is zeroed out.

Here's how the shadow memory is initialized shortly after allocation:

   0x00007f4a55fae9bd <+112>:	movl   $0xf1f1f1f1,0x7fff8000(%r12)
   0x00007f4a55fae9c9 <+124>:	movl   $0xf2f2f200,0x7fff8004(%r12)
   0x00007f4a55fae9d5 <+136>:	movl   $0xf2f2f2f2,0x7fff8008(%r12)
   0x00007f4a55fae9e1 <+148>:	movl   $0xf2f2f200,0x7fff800c(%r12)
   0x00007f4a55fae9ed <+160>:	movl   $0xf2f2f2f2,0x7fff8010(%r12)
   0x00007f4a55fae9f9 <+172>:	movl   $0xf2f2f200,0x7fff8014(%r12)
   0x00007f4a55faea05 <+184>:	movl   $0xf2f2f2f2,0x7fff8018(%r12)
   0x00007f4a55faea11 <+196>:	movl   $0xf2f2f200,0x7fff801c(%r12)
   0x00007f4a55faea1d <+208>:	movl   $0xf2f2f2f2,0x7fff8020(%r12)
   0x00007f4a55faea29 <+220>:	movl   $0xf2f2f200,0x7fff8024(%r12)
   0x00007f4a55faea35 <+232>:	movl   $0xf2f2f2f2,0x7fff8028(%r12)
   0x00007f4a55faea41 <+244>:	movl   $0xf3f3f3f3,0x7fff8030(%r12)

That leaves the shadow memory in this state:

(gdb) x /13x $r12 + 0x7fff8000
0x10006c7aec02:	0xf1f1f1f1	0xf2f2f200	0xf2f2f2f2	0xf2f2f200
0x10006c7aec12:	0xf2f2f2f2	0xf2f2f200	0xf2f2f2f2	0xf2f2f200
0x10006c7aec22:	0xf2f2f2f2	0xf2f2f200	0xf2f2f2f2	0xf8f8f8f8
0x10006c7aec32:	0xf3f3f3f3

This corresponds to the frame layout dumped by ASan upon crash:

  This frame has 6 object(s):
    [32, 40) 'image' -- NinePieceImage object at 0x10006c7aec02 + 7
    [96, 104) '<unknown>' -- first Length() object at 0x10006c7aec02 + 15
    [160, 168) '<unknown>' -- second Length() object at 0x10006c7aec12 + 7
    [224, 232) '<unknown>' -- third Length() object at + 0x10006c7aec12 + 15
    [288, 296) '<unknown>' -- fourth Length() object at 0x10006c7aec22 + 7
    [352, 384) '<unknown>' -- LengthBox() object that is being passed all the Length() objects, at 0x10006c7aec22 + 12

What's missing in the shadow memory initialization is a movl instruction that would null out the four bytes located at 0x7fff802c(%r12). Because of that, the 0xf8f8f8f8 value remains in that place.

In the end, the 0xf8f8f8f8 value is in shadow memory in a location that is associated with the LengthBox object or, more precisely, the std::array<Length, 4> member object into which the temporary Length objects are being moved. As the first move constructor is called and data store is attempted, ASan finds the 0xf8 value in the corresponding shadow memory and things go sideways.

I assume ASan expects the allocated shadow memory to be nulled-out, given the initialization instructions that are generated. That's not happening, apparently. For that reason I would mark this down as an issue with ASan, and it should be reported to the GCC project.

[Michael Catanzaro]

(In reply to Zan Dobersek from comment #24)
> I assume ASan expects the allocated shadow memory to be nulled-out, given
> the initialization instructions that are generated. That's not happening,
> apparently. For that reason I would mark this down as an issue with ASan,
> and it should be reported to the GCC project.

OK, thanks very much for this detailed analysis, Zan.

I spent some time yesterday trying to put together an isolated reproducer that would be suitable for a GCC bug report; however, I was not successful. I will keep trying.

[Michael Catanzaro]

Created attachment 343916 [details]
Patch

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #26)
> Created attachment 343916 [details]
> Patch

Zan, Darin, are you OK with landing this workaround (provided that we additionally report this issue to either the asan or the GCC developers)?

I've failed to create a reduction that doesn't depend on the WebKit codebase, but we can at least submit the complete preprocessed source of StyleBuilder.cpp (the source file that is including StyleBuilderCustom.h here).

[Darin Adler]

(In reply to Michael Catanzaro from comment #23)
>      template<typename U>
>      RectEdges(U&& top, U&& right, U&& bottom, U&& left)
> -        : m_sides({ { std::forward<T>(top), std::forward<T>(right),
> std::forward<T>(bottom), std::forward<T>(left) } })
> +        : m_sides({ { std::forward<U>(top), std::forward<U>(right),
> std::forward<U>(bottom), std::forward<U>(left) } })

Yes, I think that’s a real bug that needs to be fixed!

> And then I found some misplaced WTFMoves (moving from copies rather than
> rvalue references).

Those don’t seem like bugs. It makes sense to move from a copy, to avoid making yet another copy when doing the assignment.

[Darin Adler]

Comment on attachment 343916 [details]
Patch

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

> Source/WebCore/css/StyleBuilderCustom.h:579
> +            // FIXME: This should not be constructed when unneeded, but see https://bugs.webkit.org/show_bug.cgi?id=186980.

I’d rather have the comment do something other than pointing to the bug report. I would write this comment instead.

    // FIXME: Put this into a local variable to work around a bug in the GCC <xxx> version of Address Sanitizer.
    // Might be slightly less efficient when the type is not BorderImage since this is unused in that case.

You can reference the bug number if you like, but I’d like the comment in the code to be more specific.

[Darin Adler]

(In reply to Michael Catanzaro from comment #27)
> I've failed to create a reduction that doesn't depend on the WebKit
> codebase, but we can at least submit the complete preprocessed source of
> StyleBuilder.cpp (the source file that is including StyleBuilderCustom.h
> here).

This is just intellectual curiosity, nothing required of course: Rather than creating a reduction by writing fresh code, did you try starting with the complete preprocessed StyleBuilder.cpp and then delete as much as you could? Might be able to make a smaller test case that way.

[David Kilzer (:ddkilzer)]

(In reply to Darin Adler from comment #30)
> (In reply to Michael Catanzaro from comment #27)
> > I've failed to create a reduction that doesn't depend on the WebKit
> > codebase, but we can at least submit the complete preprocessed source of
> > StyleBuilder.cpp (the source file that is including StyleBuilderCustom.h
> > here).
> 
> This is just intellectual curiosity, nothing required of course: Rather than
> creating a reduction by writing fresh code, did you try starting with the
> complete preprocessed StyleBuilder.cpp and then delete as much as you could?
> Might be able to make a smaller test case that way.

There is a tool called “delta” that does this for you.  You just write a shell script that returns a particular status when the test input file reproduces the issue, and the delta tool then reduces the input file.

http://delta.tigris.org/

[Michael Catanzaro]

(In reply to Darin Adler from comment #28)
> > And then I found some misplaced WTFMoves (moving from copies rather than
> > rvalue references).
> 
> Those don’t seem like bugs. It makes sense to move from a copy, to avoid
> making yet another copy when doing the assignment.

Yes, I was wrong.

(In reply to Darin Adler from comment #30)
> This is just intellectual curiosity, nothing required of course: Rather than
> creating a reduction by writing fresh code, did you try starting with the
> complete preprocessed StyleBuilder.cpp and then delete as much as you could?
> Might be able to make a smaller test case that way.

Good idea.

(In reply to David Kilzer (:ddkilzer) from comment #31)
> There is a tool called “delta” that does this for you.  You just write a
> shell script that returns a particular status when the test input file
> reproduces the issue, and the delta tool then reduces the input file.
> 
> http://delta.tigris.org/

Interesting!

[Michael Catanzaro]

(In reply to Darin Adler from comment #28)
> Yes, I think that’s a real bug that needs to be fixed!

Bug #187182

[Michael Catanzaro]

Created attachment 344030 [details]
Complete preprocessed source

[Michael Catanzaro]

Created attachment 344031 [details]
Corresponding assembly

[Michael Catanzaro]

Committed r233405: <https://trac.webkit.org/changeset/233405>

[Michael Catanzaro]

GCC bug report: https://bugzilla.redhat.com/show_bug.cgi?id=1607459

I never got a reduction, so it might not be appreciated, but that will have to suffice.