UNADDRs beyond TOS in WebCore::RenderStyle::resetBorder*: compiler reads beyond TOS before setting up frame!

[derekbruening]

From bruen...@google.com on September 07, 2011 11:48:09

on chrome Release build (with /Ob0 /Oy-):

seen in several ui_tests:
test-name=OptionsUITest.LoadOptionsByURL
test-name=AutomationProxyTest.AcceleratorExtensions
test-name=OptionsUITest.NavBarCheck

Error #12476: UNADDRESSABLE ACCESS: reading 0x0037e764-0x0037e768 4 byte(s)
@0:29:15.135 in thread 3124
Note: instruction: mov 0xfffffffc(%esp) -> %eax
#0 chrome.dll!WebCore::RenderStyle::resetBorderTop [e:\src\chromium\src\third_party\webkit\source\webcore\rendering\style\renderstyle.h:845]
#1 chrome.dll!WebCore::RenderTheme::adjustRadioStyle [e:\src\chromium\src\third_party\webkit\source\webcore\rendering\rendertheme.cpp:875]
#2 chrome.dll!WebCore::RenderTheme::adjustStyle [e:\src\chromium\src\third_party\webkit\source\webcore\rendering\rendertheme.cpp:238]
#3 chrome.dll!WebCore::CSSStyleSelector::adjustRenderStyle [e:\src\chromium\src\third_party\webkit\source\webcore\css\cssstyleselector.cpp:1989]
#4 chrome.dll!WebCore::CSSStyleSelector::styleForElement [e:\src\chromium\src\third_party\webkit\source\webcore\css\cssstyleselector.cpp:1553]
#5 chrome.dll!WebCore::Node::styleForRenderer [e:\src\chromium\src\third_party\webkit\source\webcore\dom\node.cpp:1476]
#6 chrome.dll!WebCore::NodeRendererFactory::createRendererAndStyle [e:\src\chromium\src\third_party\webkit\source\webcore\dom\noderenderingcontext.cpp:317]
#7 chrome.dll!WebCore::NodeRendererFactory::createRendererIfNeeded [e:\src\chromium\src\third_party\webkit\source\webcore\dom\noderenderingcontext.cpp:366]
#8 chrome.dll!WebCore::Node::createRendererIfNeeded [e:\src\chromium\src\third_party\webkit\source\webcore\dom\node.cpp:1465]
#9 chrome.dll!WebCore::Element::attach [e:\src\chromium\src\third_party\webkit\source\webcore\dom\element.cpp:1015]
#10 chrome.dll!WebCore::HTMLFormControlElement::attach [e:\src\chromium\src\third_party\webkit\source\webcore\html\htmlformcontrolelement.cpp:155]
#11 chrome.dll!WebCore::HTMLInputElement::attach [e:\src\chromium\src\third_party\webkit\source\webcore\html\htmlinputelement.cpp:872]
#12 chrome.dll!WebCore::ContainerNode::attach [e:\src\chromium\src\third_party\webkit\source\webcore\dom\containernode.cpp:767]
#13 chrome.dll!WebCore::Element::attach [e:\src\chromium\src\third_party\webkit\source\webcore\dom\element.cpp:1021]
#14 chrome.dll!WebCore::ContainerNode::attach [e:\src\chromium\src\third_party\webkit\source\webcore\dom\containernode.cpp:767]
#15 chrome.dll!WebCore::Element::attach [e:\src\chromium\src\third_party\webkit\source\webcore\dom\element.cpp:1021]
#16 chrome.dll!WebCore::ContainerNode::attach [e:\src\chromium\src\third_party\webkit\source\webcore\dom\containernode.cpp:767]
#17 chrome.dll!WebCore::Element::attach [e:\src\chromium\src\third_party\webkit\source\webcore\dom\element.cpp:1021]
#18 chrome.dll!WebCore::ContainerNode::attach [e:\src\chromium\src\third_party\webkit\source\webcore\dom\containernode.cpp:767]
#19 chrome.dll!WebCore::Element::attach [e:\src\chromium\src\third_party\webkit\source\webcore\dom\element.cpp:1021]

other near-identical ones w/ same beyond-TOS address but
resetBorderRight, resetBorderBottom, and resetBorderLeft

void resetBorderTop() { SET_VAR(surround, border.m_top, BorderValue()) }
void resetBorderRight() { SET_VAR(surround, border.m_right, BorderValue()) }
void resetBorderBottom() { SET_VAR(surround, border.m_bottom, BorderValue()) }
void resetBorderLeft() { SET_VAR(surround, border.m_left, BorderValue()) }

#define SET_VAR(group, variable, value)
if (!compareEqual(group->variable, value))
group.access()->variable = value;

check out the disasm:
0:025> Uf chrome_68780000!WebCore::RenderStyle::resetBorderTop
chrome_68780000!WebCore::RenderStyle::resetBorderTop [e:\src\chromium\src\third_party\webkit\source\webcore\rendering\style\renderstyle.h @ 845]:
845 6977e6c0 8b4424fc mov eax,[esp-0x4]
845 6977e6c4 83ec0c sub esp,0xc
845 6977e6c7 56 push esi
845 6977e6c8 8bf1 mov esi,ecx
845 6977e6ca 8b4e18 mov ecx,[esi+0x18]
845 6977e6cd 8b9184000000 mov edx,[ecx+0x84]
845 6977e6d3 83c17c add ecx,0x7c
845 6977e6d6 250300ffff and eax,0xffff0003
845 6977e6db 57 push edi

(hmmm, I should run w/ sym+offs, it would have shown +0)

wow, I've never seen a compiler produce this before: access beyond TOS
(skipping retaddr) to get 1st param, before setting up new frame. on *nix
this would be unsafe (signals). on windows this still doesn't feel right,
though guard page handled in kernel w/o touching app stack I suppose, and
other fault on reading stack would result in failure to set up exception
stack frame anyway (no alt stack on windows).

the question is, should this just be suppressed for chrome, or since cl
seems to generate this, should drmem allow beyond-TOS on function entry (on
windows only)?

Original issue: http://code.google.com/p/drmemory/issues/detail?id=582

[derekbruening]

From bruen...@google.com on September 07, 2011 14:09:30

brain cramp: esp-4 is not a param. have not yet figured out what it's doing there.

chrome_68780000!WebCore::RenderStyle::resetBorderTop [e:\src\chromium\src\third_party\webkit\source\webcore\rendering\style\renderstyle.h @ 845]:
845 6977e6c0 8b4424fc mov eax,[esp-0x4] <== read beyond TOS!
845 6977e6c4 83ec0c sub esp,0xc <=== what is this space for? unused!
845 6977e6c7 56 push esi
845 6977e6c8 8bf1 mov esi,ecx
845 6977e6ca 8b4e18 mov ecx,[esi+0x18] <== WebCore::RenderStyle::surround
845 6977e6cd 8b9184000000 mov edx,[ecx+0x84] <== m_quirk?
845 6977e6d3 83c17c add ecx,0x7c
845 6977e6d6 250300ffff and eax,0xffff0003
845 6977e6db 57 push edi
845 6ba7e6dc 83c803 or eax,0x3 <== bottom 16 bits now set
845 6ba7e6df 8bfa mov edi,edx
845 6ba7e6e1 33f8 xor edi,eax
845 6ba7e6e3 f7c7ff0f0000 test edi,0xfff <== reading bottom only
845 6ba7e6e9 7515 jnz chrome_6aa80000!WebCore::RenderStyle::resetBorderTop+0x40 (6ba7e700)

bottom bits of esp-4 value are re-defined.
quick look doesn't show top bits being read.
no uninits are reported in this function.
instance in windbg had 0 in esp-4.
very bizarre: why read from memory at all?
caller calls this function in normal manner.
some weird artifact of cl /O2 /Ob0 /Oy- ?

going to suppress in chromium

Summary: UNADDRs beyond TOS in WebCore::RenderStyle::resetBorder*: compiler reads beyond TOS before setting up frame!

[derekbruening]

From bruen...@google.com on October 29, 2012 11:42:48

xref issue #1054

[derekbruening]

From bruen...@google.com on October 29, 2012 11:42:58

make that issue #1054