Bug 187468

Summary: valgrind claims memcpy overlap in CSSPropertyParser.cpp
Product: WebKit Reporter: Milan Crha <mcrha>
Component: CSSAssignee: Nobody <webkit-unassigned>
Status: RESOLVED INVALID    
Severity: Normal CC: ap, bugs-noreply, calvaris, mcatanzaro
Priority: P2    
Version: Other   
Hardware: Unspecified   
OS: Unspecified   

Description Milan Crha 2018-07-09 07:59:44 PDT 
I noticed this claim (among of several other) with webkit2gtk3-2.20.3-1.fc28.x86_64:

==2082== Source and destination overlap in memcpy_chk(0x1ffeffbbd7, 0x1ffeffbbd6, 8)
==2082==    at 0x4C36490: __memcpy_chk (vg_replace_strmem.c:1581)
==2082==    by 0x602E331: UnknownInlinedFun (string_fortified.h:40)
==2082==    by 0x602E331: cssValueKeywordID<char16_t> (CSSPropertyParser.cpp:184)
==2082==    by 0x602E331: WebCore::cssValueKeywordID(WTF::StringView) (CSSPropertyParser.cpp:202)
==2082==    by 0x602E495: WebCore::CSSParserToken::id() const [clone .part.152] (CSSParserToken.cpp:309)
==2082==    by 0x60501DB: WebCore::CSSPropertyParser::consumeCSSWideKeyword(WebCore::CSSPropertyID, bool) (CSSPropertyParser.cpp:342)
==2082==    by 0x6058B08: WebCore::CSSPropertyParser::parseValueStart(WebCore::CSSPropertyID, bool) (CSSPropertyParser.cpp:302)
==2082==    by 0x6058DD9: WebCore::CSSPropertyParser::parseValue(WebCore::CSSPropertyID, bool, WebCore::CSSParserTokenRange const&, WebCore::CSSParserContext const&, WTF::Vector<WebCore::CSSProperty, 256ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&, WebCore::StyleRuleBase::Type) (CSSPropertyParser.cpp:267)
==2082==    by 0x60220FD: WebCore::CSSParserImpl::consumeDeclarationValue(WebCore::CSSParserTokenRange, WebCore::CSSPropertyID, bool, WebCore::StyleRuleBase::Type) (CSSParserImpl.cpp:857)
==2082==    by 0x602677E: WebCore::CSSParserImpl::consumeDeclaration(WebCore::CSSParserTokenRange, WebCore::StyleRuleBase::Type) (CSSParserImpl.cpp:840)
==2082==    by 0x6027DA9: WebCore::CSSParserImpl::consumeDeclarationList(WebCore::CSSParserTokenRange, WebCore::StyleRuleBase::Type) (CSSParserImpl.cpp:778)
==2082==    by 0x6029094: WebCore::CSSParserImpl::consumeStyleRule(WebCore::CSSParserTokenRange, WebCore::CSSParserTokenRange) (CSSParserImpl.cpp:747)
==2082==    by 0x602956E: WebCore::CSSParserImpl::consumeQualifiedRule(WebCore::CSSParserTokenRange&, WebCore::CSSParserImpl::AllowedRulesType) (CSSParserImpl.cpp:473)
==2082==    by 0x602A331: consumeRuleList<WebCore::CSSParserImpl::parseStyleSheet(const WTF::String&, const WebCore::CSSParserContext&, WebCore::StyleSheetContents*, WebCore::CSSParser::RuleParsing)::<lambda(WTF::RefPtr<WebCore::StyleRuleBase>)> > (CSSParserImpl.cpp:387)
==2082==    by 0x602A331: WebCore::CSSParserImpl::parseStyleSheet(WTF::String const&, WebCore::CSSParserContext const&, WebCore::StyleSheetContents*, WebCore::CSSParser::RuleParsing) (CSSParserImpl.cpp:245)

The other claims are from /usr/lib64/libjavascriptcoregtk-4.0.so.18.7.11 (I do have webkit2gtk3-jsc-debuginfo-2.20.3-1.fc28.x86_64 installed, but it still shows only the library file for some reason) about invalid writes and use of uninitialized memory, but I do not want to mix it here. If you'd like a new bug for those, even without line numbers, then I can file it, but I'm afraid missing line numbers is a pita, thus I didn't do it.
Comment 1 Michael Catanzaro 2018-07-09 08:16:05 PDT 
This is memmove, not memcpy, so the source and destination are allowed to overlap....
Comment 2 Milan Crha 2018-07-09 08:21:02 PDT 
Do you mean this had been addressed after 2.20.3 release already? That would be fine then.
Comment 3 Alexey Proskuryakov 2018-07-09 14:27:40 PDT 
No, the code in question didn't change for the last couple years.

        if (isAppleLegacyCssValueKeyword(buffer, length) || hasPrefix(buffer, length, "-khtml-")) {
            memmove(buffer + 7, buffer + 6, length + 1 - 6); // This is line 184
            memcpy(buffer, "-webkit", 7);
            ++length;
        }
Comment 4 Michael Catanzaro 2018-07-10 02:56:42 PDT 
This seems to be a valgrind bug. I see the memmove() definition in glibc calls GCC's __builtin___memmove_chk().  valgrind seems to be intercepting that with __memcpy_chk rather than __memmove_chk like it should be... dunno how that's going wrong. I would report this to the valgrind developers and allow them to figure it out.