Dr. Memory doesn't handle stack expansion properly

[derekbruening]

From [email protected] on September 07, 2010 10:25:04

http://build.chromium.org/buildbot/waterfall.fyi/builders/Windows%20Tests%20(DrMemory)/builds/2594/steps/memory%20test:%20remoting/logs/stdio 01:35:51 drmemory_analyze.py [ERROR]
UNADDRESSABLE ACCESS: writing 0x0011f984-0x0011f988 4 byte(s)
#1 0x006c84cf file_util::ReadFileToString base\file_util.cc:153
#2 0x0070c32d remoting::JsonHostConfig::Read remoting\host\json_host_config.cc:27
#3 0x004591fc remoting::JsonHostConfigTest_Read_Test::TestBody remoting\host\json_host_config_unittest.cc:68

01:35:51 drmemory_analyze.py [ERROR]
UNADDRESSABLE ACCESS: writing 0x0011f950-0x0011f954 4 byte(s)
#1 0x006c84cf file_util::ReadFileToString base\file_util.cc:153
#2 0x0070c32d remoting::JsonHostConfig::Read remoting\host\json_host_config.cc:27
#3 0x0045a32c remoting::JsonHostConfigTest_Write_Test::TestBody remoting\host\json_host_config_unittest.cc:92

01:35:51 drmemory_analyze.py [ERROR]
UNADDRESSABLE ACCESS: writing 0x0011f950-0x0011f954 4 byte(s)
#1 0x006c84cf file_util::ReadFileToString base\file_util.cc:153
#2 0x0070c32d remoting::JsonHostConfig::Read remoting\host\json_host_config.cc:27
#3 0x0045a52c remoting::JsonHostConfigTest_Write_Test::TestBody remoting\host\json_host_config_unittest.cc:106

I've run the test under debugger and ESP changes its value from 0x0012F99C to 0x0011F980 when execution enters ReadFileToString function.
This may be related to issue #47 since the same test crashed a in a few seconds after these false reports.

I'm investigating these reports now and will add more details later.

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

[derekbruening]

From [email protected] on September 07, 2010 08:34:22

r46 on Windows,
===test.cpp===
#include <assert.h>
#include <stdio.h>

#include

void foo(std::string *s) {
FILE *fp = _fsopen("test.cpp", "rb", _SH_DENYNO);
assert(fp);
char buf[1024] = "";
fread(buf, 1, sizeof(buf), fp);
*s = buf;
fclose(fp);
}

void bar() {
std::string s;
foo(&s);
}

int main() {
char buf[65000] = "";
__try {
bar();
} __except(1) {
return 1;
}
return 0;
}

cl /Zi /nologo test.cpp && drmemory.exe -- test.exe

-> 4 UNADDRESSABLE ACCESSES:

Error #1: UNADDRESSABLE ACCESS: writing 0x00120160-0x00120164 4 byte(s)
@0:00:02.640 in thread 5268
0x00425c5e <test.exe+0x25c5e> test.exe!_chkstk
F:\dd\vctools\crt_bld\SELF_X86\crt\src\intel\chkstk.asm:93
0x00403d6b <test.exe+0x3d6b> test.exe!__tmainCRTStartup
f:\dd\vctools\crt_bld\self_x86\crt\src\crt0.c:327
0x7c817077 <KERNEL32.dll+0x17077> KERNEL32.dll!RegisterWaitForInputIdle
??:0

Error #2: UNADDRESSABLE ACCESS: writing 0x00120168-0x00120169 1 byte(s)
@0:00:02.672 in thread 5268
0x004012c5 <test.exe+0x12c5> test.exe!main
z:\dr-sandbox\issues\string_ref_arg\test.cpp:21
0x00403d6b <test.exe+0x3d6b> test.exe!__tmainCRTStartup
f:\dd\vctools\crt_bld\self_x86\crt\src\crt0.c:327
0x7c817077 <KERNEL32.dll+0x17077> KERNEL32.dll!RegisterWaitForInputIdle
??:0

Error #3: UNADDRESSABLE ACCESS: writing 0x00120169-0x0012016a 1 byte(s)
@0:00:03.437 in thread 5268
0x100df020 <drmemorylib.dll+0xdf020> drmemorylib.dll!replace_memset
z:\drmemory\git_svn\drmemory\replace.c:107
0x004012de <test.exe+0x12de> test.exe!main
z:\dr-sandbox\issues\string_ref_arg\test.cpp:21
0x00403d6b <test.exe+0x3d6b> test.exe!__tmainCRTStartup
f:\dd\vctools\crt_bld\self_x86\crt\src\crt0.c:327
0x7c817077 <KERNEL32.dll+0x17077> KERNEL32.dll!RegisterWaitForInputIdle
??:0

Error #4: UNADDRESSABLE ACCESS: writing 0x001537a0-0x001537a4 4 byte(s)
@0:00:05.015 in thread 5268
Note: next higher malloc: 0x001537c8-0x001537e8
Note: prev lower malloc: 0x00153548-0x0015375c
0x7c9115b9 <ntdll.dll+0x115b9> ntdll.dll!RtlInitializeCriticalSectionAndSpinCount
??:0
0x7c80b8de <KERNEL32.dll+0xb8de> KERNEL32.dll!InitializeCriticalSectionAndSpinCount
??:0
0x0040fbb3 <test.exe+0xfbb3> test.exe!__crtInitCritSecAndSpinCount
f:\dd\vctools\crt_bld\self_x86\crt\src\initcrit.c:161
0x0040af33 <test.exe+0xaf33> test.exe!_mtinitlocknum
f:\dd\vctools\crt_bld\self_x86\crt\src\mlock.c:288
0x00407481 <test.exe+0x7481> test.exe!_getstream
f:\dd\vctools\crt_bld\self_x86\crt\src\stream.c:71
0x00402e7a <test.exe+0x2e7a> test.exe!_fsopen
f:\dd\vctools\crt_bld\self_x86\crt\src\fopen.c:61
0x004011b4 <test.exe+0x11b4> test.exe!foo
z:\dr-sandbox\issues\string_ref_arg\test.cpp:7
0x00401261 <test.exe+0x1261> test.exe!bar
z:\dr-sandbox\issues\string_ref_arg\test.cpp:17
0x004012ed <test.exe+0x12ed> test.exe!main
z:\dr-sandbox\issues\string_ref_arg\test.cpp:23
0x00403d6b <test.exe+0x3d6b> test.exe!__tmainCRTStartup
f:\dd\vctools\crt_bld\self_x86\crt\src\crt0.c:327
0x7c817077 <KERNEL32.dll+0x17077> KERNEL32.dll!RegisterWaitForInputIdle
??:0

I saw a number of "RtlInitializeCriticalSectionAndSpinCount" unaddrs on Chromium tests before but couldn't reproduce them stand-alone.
__try {
run_tests()
} __except(1) {}
is what googletest does, now it makes sense why I couldn't create some of the repros before.

This is probably related to issue #11 and issue #22

Owner: derek.bruening
Labels: -Priority-Medium Priority-High

[derekbruening]

From [email protected] on September 07, 2010 08:41:17

Ooops - this gives false reports even without __try/__except!

No warnings if buf in main() is just 65 bytes.

[derekbruening]

From [email protected] on September 21, 2010 04:55:53

Derek,
What's the status of the bug?

[derekbruening]

From [email protected] on September 22, 2010 20:05:15

I ran into this independently and have a fix, should verify on your test app though.
the problem is NtQueryVirtualMemory behaving in a way I thought it only did for free memory: switching to a query loop.

[derekbruening]

From [email protected] on September 24, 2010 09:52:43

my experiments show that mbi.BaseAddress is ALWAYS just PAGE_START(addr)
whether for an image, data file, or anon memory, on xp64, xp32, or win2k,
and is thus useless.
this is surprising: I remember noticing that for MEM_FREE the BaseAddress
had this behavior, but all this time I was under the impression that it was
the real base of that same-prot region -- certainly the Win32 API routine
docs imply it is. I checked calling VirtualQuery() and I get the same
results, and it looks like it is just a wrapper. Crazy.
inside DR we usually call get_memory_info() just for prot, but there are
some places where we use the base, and we'll get it wrong.
so rather than my Dr. Memory-specific fix it seems that not only the
API-facing dr_query_memory() but also the internal get_memory_info() must
be changed inside DR.

some examples:
dr_query_memory WRONG: 0x00011005 inside 0x00010000-0x00012000 => 0x00011000-0x00012000
dr_query_memory WRONG: 0x00031005 inside 0x00030000-0x00069000 => 0x00031000-0x00069000
dr_query_memory WRONG: 0x0006a005 inside 0x00069000-0x0006b000 => 0x0006a000-0x0006b000
dr_query_memory WRONG: 0x0006c005 inside 0x0006b000-0x00070000 => 0x0006c000-0x00070000
...

[derekbruening]

From [email protected] on September 24, 2010 10:36:16

filed https://code.google.com/p/dynamorio/issues/detail?id=345 for the DynamoRIO fix

[derekbruening]

From [email protected] on September 24, 2010 16:08:41

on test.cpp pasted above:
w/o the fix:
:::Dr.Memory::: 4 unique, 65002 total unaddressable access(es)
with issue #345 fixed:
:::Dr.Memory::: 0 unique, 0 total unaddressable access(es)

Status: Fixed
Owner: [email protected]

[derekbruening]

From [email protected] on September 25, 2010 07:59:26

Thanks for fixing this!

I've uplodaded r57 / r434 to the Chromium bot and it looks like there are still some unaddr reports: http://build.chromium.org/buildbot/waterfall.fyi/builders/Windows%20Tests%20(DrMemory)/builds/2905 but they may not be unrelated to issue #49 .

I'll investigate on Monday

[derekbruening]

From [email protected] on September 27, 2010 08:24:31

Looks like these reports are still Dr. Memory bugs.
I've filed a separate issue ( r55 ) so marking this one as Verified.

Status: Verified

[derekbruening]

From [email protected] on September 27, 2010 08:24:57

Sorry, " issue #55 ", not " r55 "

[derekbruening]

From [email protected] on November 03, 2010 10:23:35

Issue 22 has been merged into this issue.

Cc: derek.bruening