Open derekbruening opened 9 years ago
From bruen...@google.com on June 22, 2011 08:49:49
This was a design decision, so this is more a feature request and not a bug. Valgrind prior to 3.0 also did not propagate through fp.
Summary: propagate shadow values through floating-point registers
Labels: -Type-Defect -Bug-FalsePositive Type-Enhancement
From timurrrr@google.com on June 22, 2011 11:01:04
You're reporting an uninit when anything uninit goes to FPU, right?
What do you think about marking the whole FPU register as uninit if any bit of the input is uninit but not reporting?
From bruen...@google.com on June 22, 2011 11:12:56
could use reduced granularity for fp and multimedia regs, yes.
From rnk@google.com on March 21, 2012 08:30:21
We hit a similar issue in http://crbug.com/115326 and again in a copy ctor for FormDataElement in a recent full run: http://build.chromium.org/p/chromium.fyi/builders/Windows%20Tests%20%28DrMemory%20full%29/builds/1136 The reports:
Dr.M Error #1
: UNINITIALIZED READ: reading 0x002af2c0-0x002af2c8 8 byte(s)
Dr.M # 0 WebCore::FormDataElement::FormDataElement +0x5f (0x04d3bfbf <unit_tests.exe+0x401bfbf>)
Dr.M # 1 WTF::VectorWebCore::FormDataElement,0::appendSlowCaseWebCore::FormDataElement [third_party\webkit\source\javascriptcore\wtf\vector.h:1016]
Dr.M # 2 WTF::VectorWebCore::FormDataElement,0::appendWebCore::FormDataElement [third_party\webkit\source\javascriptcore\wtf\vector.h:1003]
Dr.M # 3 WebCore::FormData::appendData [third_party\webkit\source\webcore\platform\network\formdata.cpp:157]
Dr.M # 4 WebKit::WebHTTPBody::appendData [third_party\webkit\source\webkit\chromium\src\webhttpbody.cpp:116]
Dr.M # 5 SessionServiceTest_KeepPostDataWithoutPasswords_Test::TestBody [chrome\browser\sessions\session_service_unittest.cc:788]
Dr.M # 6 testing::internal::HandleExceptionsInMethodIfSupportedtesting::Test,void [testing\gtest\src\gtest.cc:2145]
Dr.M Note: @0:11:51.784 in thread 1556
Dr.M Note: instruction: fld 0x28(%ecx) -> %st0
Dr.M
Dr.M Error #2
: UNINITIALIZED READ: reading 0x002aef14-0x002aef1c 8 byte(s)
Dr.M # 0 WebCore::FormDataElement::FormDataElement +0x5f (0x04d3bfbf <unit_tests.exe+0x401bfbf>)
Dr.M # 1 WTF::VectorWebCore::FormDataElement,0::appendSlowCaseWebCore::FormDataElement [third_party\webkit\source\javascriptcore\wtf\vector.h:1016]
Dr.M # 2 WTF::VectorWebCore::FormDataElement,0::appendWebCore::FormDataElement [third_party\webkit\source\javascriptcore\wtf\vector.h:1003]
Dr.M # 3 WebCore::FormData::appendData [third_party\webkit\source\webcore\platform\network\formdata.cpp:157]
Dr.M # 4 WebKit::WebHTTPBody::appendData [third_party\webkit\source\webkit\chromium\src\webhttpbody.cpp:116]
Dr.M # 5 webkit_glue::anonymous namespace'::ReadFormData [webkit\glue\glue_serialize.cc:289] \~~Dr.M~~ # 6 webkit_glue::
anonymous namespace'::ReadHistoryItem [webkit\glue\glue_serialize.cc:424]
Dr.M # 7 webkit_glue::anonymous namespace'::HistoryItemFromString [webkit\glue\glue_serialize.cc:468] \~~Dr.M~~ # 8 webkit_glue::RemovePasswordDataFromHistoryState [webkit\glue\glue_serialize.cc:491] \~~Dr.M~~ # 9 BaseSessionService::CreateUpdateTabNavigationCommand [chrome\browser\sessions\base_session_service.cc:175] \~~Dr.M~~
#10SessionService::UpdateTabNavigation [chrome\browser\sessions\session_service.cc:411] \~~Dr.M~~
#11SessionServiceTest::UpdateNavigation [chrome\browser\sessions\session_service_unittest.cc:87] \~~Dr.M~~
#12SessionServiceTest_KeepPostDataWithoutPasswords_Test::TestBody [chrome\browser\sessions\session_service_unittest.cc:808] \~~Dr.M~~
#13` testing::internal::HandleExceptionsInMethodIfSupportedtesting::Test,void [testing\gtest\src\gtest.cc:2145]
Dr.M Note: @0:11:52.003 in thread 1556
Dr.M Note: instruction: fld 0x28(%ecx) -> %st0
This is one case where it would be nice to know what the overloaded param types are, since it's not obvious that this is the copy ctor. Looking at all the ctors shows that it's in the copy ctor PC range:
0:000> x unit_tests!WebCore::FormDataElement::FormDataElement 0438bf60 unittests!WebCore::FormDataElement::FormDataElement (WebCore::FormDataElement) 0438a9f0 unit_tests!WebCore::FormDataElement::FormDataElement ( void ) 0438a8a0 unittests!WebCore::FormDataElement::FormDataElement (WebCore::KURL) 0438a820 unittests!WebCore::FormDataElement::FormDataElement (WTF::String, int64, int64, double, bool) 0438a7d0 unittests!WebCore::FormDataElement::FormDataElement (WTF::Vector<char,0>)
The assembly for it:
0:000> uf 0438bf60 unit_tests!WebCore::FormDataElement::FormDataElement: 0438bf60 55 push ebp 0438bf61 8bec mov ebp,esp 0438bf63 51 push ecx 0438bf64 894dfc mov [ebp-0x4],ecx 0438bf67 8b45fc mov eax,[ebp-0x4] 0438bf6a 8b4d08 mov ecx,[ebp+0x8] 0438bf6d 8b11 mov edx,[ecx] 0438bf6f 8910 mov [eax],edx 0438bf71 8b4508 mov eax,[ebp+0x8] 0438bf74 83c004 add eax,0x4 0438bf77 50 push eax 0438bf78 8b4dfc mov ecx,[ebp-0x4] 0438bf7b 83c104 add ecx,0x4 0438bf7e e84d5b75fd call unit_tests!WTF::Vector<char,0>::Vector<char,0> (01ae1ad0) 0438bf83 8b4d08 mov ecx,[ebp+0x8] 0438bf86 83c110 add ecx,0x10 0438bf89 51 push ecx 0438bf8a 8b4dfc mov ecx,[ebp-0x4] 0438bf8d 83c110 add ecx,0x10 0438bf90 e80b3675fd call unit_tests!WTF::String::String (01adf5a0) 0438bf95 8b55fc mov edx,[ebp-0x4] 0438bf98 8b4508 mov eax,[ebp+0x8] 0438bf9b 8b4818 mov ecx,[eax+0x18] 0438bf9e 894a18 mov [edx+0x18],ecx 0438bfa1 8b401c mov eax,[eax+0x1c] 0438bfa4 89421c mov [edx+0x1c],eax 0438bfa7 8b4dfc mov ecx,[ebp-0x4] 0438bfaa 8b5508 mov edx,[ebp+0x8] 0438bfad 8b4220 mov eax,[edx+0x20] 0438bfb0 894120 mov [ecx+0x20],eax 0438bfb3 8b5224 mov edx,[edx+0x24] 0438bfb6 895124 mov [ecx+0x24],edx 0438bfb9 8b45fc mov eax,[ebp-0x4] 0438bfbc 8b4d08 mov ecx,[ebp+0x8] 0438bfbf dd4128 fld qword ptr [ecx+0x28] 0438bfc2 dd5828 fstp qword ptr [eax+0x28] 0438bfc5 8b5508 mov edx,[ebp+0x8] 0438bfc8 83c230 add edx,0x30 0438bfcb 52 push edx 0438bfcc 8b4dfc mov ecx,[ebp-0x4] 0438bfcf 83c130 add ecx,0x30 0438bfd2 e809e677fd call unit_tests!WebCore::KURL::KURL (01b0a5e0) 0438bfd7 8b4508 mov eax,[ebp+0x8] 0438bfda 0588000000 add eax,0x88 0438bfdf 50 push eax 0438bfe0 8b4dfc mov ecx,[ebp-0x4] 0438bfe3 81c188000000 add ecx,0x88 0438bfe9 e8b23575fd call unit_tests!WTF::String::String (01adf5a0) 0438bfee 8b4dfc mov ecx,[ebp-0x4] 0438bff1 8b5508 mov edx,[ebp+0x8] 0438bff4 8a828c000000 mov al,[edx+0x8c] 0438bffa 88818c000000 mov [ecx+0x8c],al 0438c000 8b45fc mov eax,[ebp-0x4] 0438c003 8be5 mov esp,ebp 0438c005 5d pop ebp 0438c006 c20400 ret 0x4
The key being these instructions: 0438bfbf dd4128 fld qword ptr [ecx+0x28] 0438bfc2 dd5828 fstp qword ptr [eax+0x28]
Just propagating the bits through registers would fix this.
From rnk@google.com on December 12, 2012 12:49:16
I hit another instance of this in layout tests. Here's the full report:
UNINITIALIZED READ: reading 0x0018ea44-0x0018ea4c 8 byte(s)
#20 base::internal::RunnableAdapter<void (__thiscall`anonymous namespace'::RequestProxy::*)(int,std::basic_string<char,std::char_traits
What's happening in this case is someone is using a hashmap or InitiatorInfo structs, which has a double field.
struct InitiatorInfo {
AtomicString name;
double startTime;
};
So it looks like some part of the hashtable implementation is asking for emptyValue of the key-value pair type, which is just a default initialized struct. The default empty ctor leaves it uninitialized. Then it calls the copy constructor on it, where we report the bug.
Suppressing this in chrome.
From bruen...@google.com on May 20, 2013 13:15:19
xref issue #1188 where it looks like this is causing tons of false pos on spec2k6 wrf
xref issue #931
Labels: Bug-FalsePositive
From bruen...@google.com on December 18, 2013 22:19:44
Another instance: http://build.chromium.org/p/chromium.fyi/builders/Windows%20Unit%20%28DrMemory%20full%29%20%282%29/builds/1084/steps/memory%20test%3A%20unit/logs/stdio [ RUN ] FieldMapWrapperTest.BothShippingAndBillingCanCoexist
Dr.M
Dr.M Error #3
: UNINITIALIZED READ: reading 0x0018f86c-0x0018f870 4 byte(s)
Dr.M # 0 autofill::DetailInput::DetailInput
Dr.M # 1 std::_Construct<autofill::DetailInput,autofill::DetailInput const &> [c:\program files (x86)\microsoft visual studio 10.0\vc\include\xmemory:48]
Dr.M # 2 std::allocatorautofill::DetailInput::construct [c:\program files (x86)\microsoft visual studio 10.0\vc\include\xmemory:197]
Dr.M # 3 std::_Cons_valstd::allocator<autofill::DetailInput,autofill::DetailInput,autofill::DetailInput const &> [c:\program files (x86)\microsoft visual studio 10.0\vc\include\xmemory:280]
Dr.M # 4 std::vectorautofill::DetailInput,std::allocatorDr.M # 5 autofill::FieldMapWrapperTest_BothShippingAndBillingCanCoexist_Test::TestBody [chrome\browser\ui\autofill\data_model_wrapper_unittest.cc:176]
Dr.M # 6 testing::internal::HandleExceptionsInMethodIfSupportedtesting::Test,void [testing\gtest\src\gtest.cc:2045]
Dr.M Note: @0:38:26.918 in thread 3744
Dr.M Note: instruction: fld 0x0c(%ecx) -> %st0
Dr.M
Dr.M Error #4
: UNINITIALIZED READ: reading 0x0018f8c0-0x0018f8c4 4 byte(s)
Dr.M # 0 autofill::DetailInput::DetailInput
Dr.M # 1 std::_Construct<autofill::DetailInput,autofill::DetailInput const &> [c:\program files (x86)\microsoft visual studio 10.0\vc\include\xmemory:48]
Dr.M # 2 std::allocatorautofill::DetailInput::construct [c:\program files (x86)\microsoft visual studio 10.0\vc\include\xmemory:197]
Dr.M # 3 std::_Cons_valstd::allocator<autofill::DetailInput,autofill::DetailInput,autofill::DetailInput const &> [c:\program files (x86)\microsoft visual studio 10.0\vc\include\xmemory:280]
Dr.M # 4 std::vectorautofill::DetailInput,std::allocatorDr.M # 5 autofill::FieldMapWrapperTest_BothShippingAndBillingCanCoexist_Test::TestBody [chrome\browser\ui\autofill\data_model_wrapper_unittest.cc:180]
Dr.M # 6 testing::internal::HandleExceptionsInMethodIfSupportedtesting::Test,void [testing\gtest\src\gtest.cc:2045]
Dr.M Note: @0:38:26.918 in thread 3744
Dr.M Note: instruction: fld 0x0c(%ecx) -> %st0
[ OK ] FieldMapWrapperTest.BothShippingAndBillingCanCoexist (453 ms)
0213d390 unit_tests!autofill::DetailInput::DetailInput (struct autofill::DetailInput *) 0213c7e0 unit_tests!autofill::DetailInput::DetailInput (void)
We have a default copy constructor copying from a default-constructor-created instance.
TEST(FieldMapWrapperTest, BothShippingAndBillingCanCoexist) { DetailInputs inputs;
DetailInput billing_street; billing_street.type = ADDRESS_BILLING_STREET_ADDRESS; inputs.push_back(billing_street);
172 0213c451 8d8d74ffffff lea ecx,[ebp-8Ch] 172 0213c457 e81dc435fe call unit_tests!ILT+620660(??0?$vectorUDetailInputautofillV?$allocatorUDetailInputautofillstdstdQAEXZ) (00498879) 172 0213c45c c745fc00000000 mov dword ptr [ebp-4],0 174 0213c463 8d8d38ffffff lea ecx,[ebp-0C8h] 174 0213c469 e864055afe call unit_tests!ILT+2996685(??0DetailInputautofillQAEXZ) (006dc9d2) 174 0213c46e c645fc01 mov byte ptr [ebp-4],1 175 0213c472 c7853cffffff4e000000 mov dword ptr [ebp-0C4h],4Eh 176 0213c47c 8d8538ffffff lea eax,[ebp-0C8h] 176 0213c482 50 push eax 176 0213c483 8d8d74ffffff lea ecx,[ebp-8Ch] 176 0213c489 e8a9e44cfe call unit_tests!ILT+2136370(?push_back?$vectorUDetailInputautofillV?$allocatorUDetailInputautofillstdstdQAEXABUDetailInputautofillZ) (0060a937)
The test just uses the "type" field and leaves the other fields, including a float, alone. These constructors are not initializing the floating-point field, but nothing "significant" is done with the field.
From bruen...@google.com on December 19, 2013 08:41:02
Just to add a little more clarity to the last post: the code is initializing and copying a struct that has a float field. The default constructor leaves the field alone but the copy constructor copies it with this familiar pattern:
0213d3bf d9410c fld dword ptr [ecx+0Ch] 0213d3c2 d9580c fstp dword ptr [eax+0Ch]
Until we have full shadowing and propagation, perhaps we can add a heuristic to handle copy constructors that looks for "fld;fst"?
From bruen...@google.com on January 15, 2014 08:14:29
The short-term plan is to add a heuristic that, on an uninit in an fld, if there's a subsequent fstp, does a slowpath propagation. We considered trying to work that into the fastpath, but that gets complex (as it also has to support the slowpath, and so can't rely on just IR notes), and we don't support 8-byte mem2mem propagation in any case -- so we're going with slowpath handling of this case.
Long-term we'll implement the actual fp reg shadowing. Here are some thoughts:
\ TODO could use reduced granularity for fp and multimedia regs
For fp, 2 bits per each of 8 st* registers => only 2 bytes needed. Seems reasonable, b/c memory values are converted to x86's extended precision format and it would be complex to try and map which bits go where.
For multimedia regs, however, it's a very straightforward 1-to-1 mapping, so it seems we'd want full shadowing.
\ TODO how track physical fp registers vs stN stack?
If an instruction references stN, we need to map it to a physical register.
Can read TOP field (bits 11..13: see Figure 8-4) of x87 FPU status word to find which register is the top.
Can get status word into %ax (or into memory) via OP_fnstsw. If store each fp reg as 2 bits inside 2-byte shadow region, we'd do something like:
fnstw ax and ax, 0x3800 shr ax, 10 # >> 11 and then *2 access fs:(fp_shadow_base + ax)
Should be able to ignore stack pops (i.e., treat OP_fstp as OP_fst).
Every bb w/ fp instrs should try to use eax as global spill (or could do fnstw to TLS).
Labels: -Priority-Low Priority-Medium
From bruen...@google.com on January 24, 2014 07:15:34
The heuristic (under internal option -fld_fstp_prop) is in r1679 and seems to work well: it addresses all the copy constructor false positives in Chromium as well as fixing issue #931 .
From bruen...@google.com on January 28, 2014 07:57:46
Chromium Release unit_tests doesn't match the heuristic:
[ RUN ] FieldMapWrapperTest.BothShippingAndBillingCanCoexist
Dr.M
Dr.M Error #1
: UNINITIALIZED READ: reading 0x0037f194-0x0037f198 4 byte(s)
Dr.M # 0 autofill::DetailInput::DetailInput
Dr.M # 1 std::_Cons_val<> [c:\program files (x86)\microsoft visual studio 10.0\vc\include\xmemory:280]
Dr.M # 2 std::vector<>::push_back [c:\program files (x86)\microsoft visual studio 10.0\vc\include\vector:995]
Dr.M # 3 autofill::FieldMapWrapperTest_BothShippingAndBillingCanCoexist_Test::TestBody [chrome\browser\ui\autofill\data_model_wrapper_unittest.cc:180]
Dr.M # 4 testing::internal::HandleExceptionsInMethodIfSupported<> [testing\gtest\src\gtest.cc:2045]
Dr.M Note: @0:34:40.217 in thread 39556
Dr.M Note: instruction: fld 0x24(%edi) -> %st0
There's an instr in between:
01f9fa49 d94724 fld dword ptr [edi+24h] 01f9fa4c 33d2 xor edx,edx 01f9fa4e d95e24 fstp dword ptr [esi+24h]
W/ new scheme of marking dst mem as bitlevel, seems like we can easily handle an instr in between that doesn't touch memory or write to fstp's base reg.
From bruen...@google.com on January 29, 2014 09:21:32
\ TODO chrome issue #471a suppression: not a copy constructor!
This one has no additional information:
UNINITIALIZED READ
name= https://code.google.com/p/drmemory/issues/detail?id=471 a
_!media::AudioRendererAlgorithmOLA::Crossfade
It's not being used on today's media_ bot (nor on any others: I checked them all) -- so should we remove it?
Most likely it's similar to Timur's minimal reproducer (what he's calling "garbage in, garbage out"):
TEST(FloatTests, Multiply) { /* The "minimal reproducer" in issue #471: _/ int value = new int; value = (int)(_value * 0.33); delete value; }
Which does result in a report (which is why I did not keep it in float_tests.cpp):
Dr.M Error #1
: UNINITIALIZED READ: reading 0x03f3d818-0x03f3d81c 4 byte(s)
Dr.M # 0 FloatTests_Multiply_Test::TestBody [d:\derek\drmemory\git\src\tests\app_suite\float_tests.cpp:84]
Its code looks like this:
TAG 0x00fcb6d0 +0 L3 83 c4 04 add $0x00000004 %esp -> %esp +3 L3 89 45 f8 mov %eax -> 0xfffffff8(%ebp) +6 L3 8b 45 f8 mov 0xfffffff8(%ebp) -> %eax +9 L3 89 45 fc mov %eax -> 0xfffffffc(%ebp) +12 L3 8b 4d fc mov 0xfffffffc(%ebp) -> %ecx +15 L3 db 01 fild (%ecx) -> %st0 +17 L3 dc 0d 28 4b 09 01 fmul 0x01094b28 %st0 -> %st0 +23 L3 e8 b4 fa 08 00 call $0x0105b1a0 %esp -> %esp 0xfffffffc(%esp) END 0x00fcb6d0
Recent build, targeting the subtests that likely had this code:
% ~/drmemory/git/build_x86_dbg/bin/drmemory.exe -dr d:/derek/dr/git/exports -batch -- ./media_unittests.exe --gtest_filter=AudioRendererAlgorithmTest.* --single-process-tests
=> no uninits at all.
\ TODO chrome Release: ipc!IPC::ParamTraits
Running Release full-mode browser_tests hits a
[ RUN ] IFrameTest.InEmptyFrame
Error #2
: UNINITIALIZED READ: reading 0x0032da40-0x0032da44 4 byte(s)
Note: @0:00:38.752 in thread 14004 Note: instruction: fld (%eax) -> %st0
0:000> Uf 10002820
ipc!IPC::ParamTraits
static void Write(Message* m, const param_type& p) { m->WriteFloat(p); }
Goes through Pickle::WriteFloat which ends up at Pickle::WriteBytesCommon() which basically calls memcpy().
\ TODO webkit!WebCore::RenderBlock::computeInlinePreferredLogicalWidths needs full fp prop
Debug browser_tests:
[ RUN ] IdentityInternalsSingleTokenWebUITest.getAllTokens
Error #3
: UNINITIALIZED READ: reading 0x0018cd5c-0x0018cd60 4 byte(s)
Note: instruction: fld 0xffffff44(%ebp) -> %st0
It needs full issue #471: webkit!WebCore::RenderBlock::computeInlinePreferredLogicalWidths+0x93b [e:\derek\chromium\src\third_party\webkit\source\core\rendering\renderblock.cpp @ 4195]: 4195 11d9e98b d945a4 fld dword ptr [ebp-5Ch] 4195 11d9e98e d88548ffffff fadd dword ptr [ebp-0B8h] 4195 11d9e994 d95da4 fstp dword ptr [ebp-5Ch] 4196 11d9e997 d98544ffffff fld dword ptr [ebp-0BCh] 4196 11d9e99d d88548ffffff fadd dword ptr [ebp-0B8h] 4196 11d9e9a3 d99d44ffffff fstp dword ptr [ebp-0BCh] 4197 11d9e9a9 c645c101 mov byte ptr [ebp-3Fh],1
From timurrrr@google.com on June 22, 2011 11:39:27
AFAIK, Dr. Memory issues a report when any uninit data goes into an FPU register.
Today I've found that results in false positive reports on a few Chromium media_unittests.
What they do in those tests is: create X bytes of random garbage -> process -> make sure the output size is still X bytes and nothing crashed. Some call it Garbage-In-Garbage-Out :)
Here's a short repro. It is Valgrind-clean but Dr. Memory report an uninit on the "fild" instruction (at line 3)
int main() { int value = new int; value = value \ 0.33; delete value; return 0; }
Original issue: http://code.google.com/p/drmemory/issues/detail?id=471