Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unaddr error from v8 generated code to be investigated #1582

Open
derekbruening opened this issue Nov 28, 2014 · 7 comments
Open

Unaddr error from v8 generated code to be investigated #1582

derekbruening opened this issue Nov 28, 2014 · 7 comments

Comments

@derekbruening
Copy link
Contributor

From [email protected] on June 26, 2014 17:34:12

http://build.chromium.org/p/chromium.fyi/builders/Windows%20Browser%20%28DrMemory%20full%29%20%286%29/builds/639 =====================================================
Below is the report for drmemory wrapper PID=2356_20.
It was used while running the UnloadTest.CrossSiteInfiniteBeforeUnloadAsync test.

Suppressions used:
count name
3 https://code.google.com/p/drmemory/issues/detail?id=18 d
3 http://crbug.com/346842 4 bug_347967_all_osmesa
4 https://code.google.com/p/drmemory/issues/detail?id=113 rpcrt4.dll wildcard
4 https://code.google.com/p/drmemory/issues/detail?id=412 h
4 https://code.google.com/p/drmemory/issues/detail?id=513 d
4 https://code.google.com/p/drmemory/issues/detail?id=748 7 http://crbug.com/346993 8 https://code.google.com/p/drmemory/issues/detail?id=512 b
13 https://code.google.com/p/drmemory/issues/detail?id=412 c
21 http://crbug.com/371942 24 https://code.google.com/p/drmemory/issues/detail?id=68 a
44 http://crbug.com/371357 770 sqlite3_randomness UNINIT

09:55:40 drmemory_analyze.py [INFO] Found 1 error reports
09:55:40 drmemory_analyze.py [INFO] Report #1
UNADDRESSABLE ACCESS: reading 0x002ee964-0x002ee968 4 byte(s)
#0 (0x3eb4f2fa)
#1 v8.dll!v8::internal::Invoke [v8\src\execution.cc:94]
#2 v8.dll!v8::internal::Execution::Call [v8\src\execution.cc:149]
#3 v8.dll!v8::Function::Call [v8\src\api.cc:4002]
#4 blink_web.dll!WebCore::V8ScriptRunner::callFunction [third_party\webkit\source\bindings\v8\v8scriptrunner.cpp:140]
#5 blink_web.dll!WebCore::ScriptController::callFunction [third_party\webkit\source\bindings\v8\scriptcontroller.cpp:162]
#6 blink_web.dll!WebCore::ScriptController::callFunction [third_party\webkit\source\bindings\v8\scriptcontroller.cpp:145]
#7 blink_web.dll!WebCore::V8EventListener::callListenerFunction [third_party\webkit\source\bindings\v8\v8eventlistener.cpp:88]
#8 blink_web.dll!WebCore::V8AbstractEventListener::invokeEventHandler [third_party\webkit\source\bindings\v8\v8abstracteventlistener.cpp:126]
#9 blink_web.dll!WebCore::V8AbstractEventListener::handleEvent [third_party\webkit\source\bindings\v8\v8abstracteventlistener.cpp:96]
#10 blink_web.dll!WebCore::EventTarget::fireEventListeners [third_party\webkit\source\core\events\eventtarget.cpp:348]
#11 blink_web.dll!WebCore::EventTarget::fireEventListeners [third_party\webkit\source\core\events\eventtarget.cpp:284]
#12 blink_web.dll!WebCore::LocalDOMWindow::dispatchEvent [third_party\webkit\source\core\frame\localdomwindow.cpp:1634]
#13 blink_web.dll!WebCore::Document::dispatchBeforeUnloadEvent [third_party\webkit\source\core\dom\document.cpp:2610]
#14 blink_web.dll!WebCore::FrameLoader::shouldClose [third_party\webkit\source\core\loader\frameloader.cpp:1275]
#15 content.dll!content::RenderFrameImpl::OnBeforeUnload [content\renderer\render_frame_impl.cc:919]
#16 content.dll!content::RenderFrameImpl::OnMessageReceived [content\renderer\render_frame_impl.cc:684]
#17 content.dll!content::MessageRouter::RouteMessage [content\common\message_router.cc:54]

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

@derekbruening
Copy link
Contributor Author

From [email protected] on June 26, 2014 14:35:33

This could be reproduced locally:

.\tools\valgrind\chrome_tests.bat -b out\Release -t browser_tests --tool drmemory_full --gtest_filter=UnloadTest.CrossSiteInfiniteBeforeUnloadAsync
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from UnloadTest, where TypeParam =
[ RUN ] UnloadTest.CrossSiteInfiniteBeforeUnloadAsync
...
Dr.M
Dr.M Error #1: UNADDRESSABLE ACCESS: reading 0x0021e83c-0x0021e840 4 byte(s)
Dr.M # 0 (0x35e4f31a)
Dr.M # 1 v8.dll!v8::internal::Invoke [d:\src\chrome-int\src\v8\src\execution.cc:94]
Dr.M # 2 v8.dll!v8::internal::Execution::Call [d:\src\chrome-int\src\v8\src\execution.cc:149]
Dr.M # 3 v8.dll!v8::Function::Call [d:\src\chrome-int\src\v8\src\api.cc:4002]
Dr.M # 4 blink_web.dll!WebCore::V8ScriptRunner::callFunction [d:\src\chrome-int\src\third_party\webkit\source\bindings\v8\v8scriptrunner.cpp:140]
Dr.M # 5 blink_web.dll!WebCore::ScriptController::callFunction [d:\src\chrome-int\src\third_party\webkit\source\bindings\v8\scriptcontroller.cpp:162]
Dr.M # 6 blink_web.dll!WebCore::ScriptController::callFunction [d:\src\chrome-int\src\third_party\webkit\source\bindings\v8\scriptcontroller.cpp:145]
Dr.M # 7 blink_web.dll!WebCore::V8EventListener::callListenerFunction [d:\src\chrome-int\src\third_party\webkit\source\bindings\v8\v8eventlistener.cpp:88]
Dr.M # 8 blink_web.dll!WebCore::V8AbstractEventListener::invokeEventHandler [d:\src\chrome-int\src\third_party\webkit\source\bindings\v8\v8abstracteventlistener.cpp:126]
Dr.M # 9 blink_web.dll!WebCore::V8AbstractEventListener::handleEvent [d:\src\chrome-int\src\third_party\webkit\source\bindings\v8\v8abstracteventlistener.cpp:96]
Dr.M #10 blink_web.dll!WebCore::EventTarget::fireEventListeners [d:\src\chrome-int\src\third_party\webkit\source\core\events\eventtarget.cpp:348]
Dr.M #11 blink_web.dll!WebCore::EventTarget::fireEventListeners [d:\src\chrome-int\src\third_party\webkit\source\core\events\eventtarget.cpp:284]
Dr.M #12 blink_web.dll!WebCore::LocalDOMWindow::dispatchEvent [d:\src\chrome-int\src\third_party\webkit\source\core\frame\localdomwindow.cpp:1634]
Dr.M #13 blink_web.dll!WebCore::Document::dispatchBeforeUnloadEvent [d:\src\chrome-int\src\third_party\webkit\source\core\dom\document.cpp:2646]
Dr.M #14 blink_web.dll!WebCore::FrameLoader::shouldClose [d:\src\chrome-int\src\third_party\webkit\source\core\loader\frameloader.cpp:1275]
Dr.M #15 content.dll!content::RenderFrameImpl::OnBeforeUnload [d:\src\chrome-int\src\content\renderer\render_frame_impl.cc:919]
Dr.M #16 content.dll!content::RenderFrameImpl::OnMessageReceived [d:\src\chrome-int\src\content\renderer\render_frame_impl.cc:684]
Dr.M #17 content.dll!content::MessageRouter::RouteMessage [d:\src\chrome-int\src\content\common\message_router.cc:54]
Dr.M #18 content.dll!content::MessageRouter::OnMessageReceived [d:\src\chrome-int\src\content\common\message_router.cc:46]
Dr.M #19 content.dll!content::ChildThread::OnMessageReceived [d:\src\chrome-int\src\content\child\child_thread.cc:467]
Dr.M #20 ipc.dll!IPC::ChannelProxy::Context::OnDispatchMessage [d:\src\chrome-int\src\ipc\ipc_channel_proxy.cc:273]
Dr.M #21 ipc.dll!base::internal::Invoker<>::Run [d:\src\chrome-int\src\base\bind_internal.h:1253]
Dr.M #22 base.dll!base::MessageLoop::RunTask [d:\src\chrome-int\src\base\message_loop\message_loop.cc:450]
Dr.M #23 base.dll!base::MessageLoop::DeferOrRunPendingTask [d:\src\chrome-int\src\base\message_loop\message_loop.cc:462]
Dr.M #24 base.dll!base::MessageLoop::DoWork [d:\src\chrome-int\src\base\message_loop\message_loop.cc:576]
Dr.M #25 base.dll!base::MessagePumpDefault::Run [d:\src\chrome-int\src\base\message_loop\message_pump_default.cc:32]
Dr.M #26 base.dll!base::MessageLoop::RunHandler [d:\src\chrome-int\src\base\message_loop\message_loop.cc:400]
Dr.M #27 content.dll!content::RendererMain [d:\src\chrome-int\src\content\renderer\renderer_main.cc:250]
Dr.M #28 content.dll!content::RunNamedProcessTypeMain [d:\src\chrome-int\src\content\app\content_main_runner.cc:416]
Dr.M #29 content.dll!content::ContentMainRunnerImpl::Run [d:\src\chrome-int\src\content\app\content_main_runner.cc:762]
Dr.M #30 content.dll!content::ContentMain [d:\src\chrome-int\src\content\app\content_main.cc:19]
Dr.M #31 content::LaunchTests [d:\src\chrome-int\src\content\public\test\test_launcher.cc:474]
Dr.M #32 LaunchChromeTests [d:\src\chrome-int\src\chrome\test\base\chrome_test_launcher.cc:124]
Dr.M #33 main [d:\src\chrome-int\src\chrome\test\base\browser_tests_main.cc:21]
Dr.M Note: @0:00:33.398 in thread 7480
Dr.M Note: instruction: push 0xfffffff4(%ebp) %esp -> %esp 0xfffffffc(%esp)
[ OK ] UnloadTest.CrossSiteInfiniteBeforeUnloadAsync (105310 ms)
[----------] 1 test from UnloadTest (105343 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (105424 ms total)
[ PASSED ] 1 test.

@derekbruening
Copy link
Contributor Author

From [email protected] on June 26, 2014 14:46:40

The corresponding asm:
35e4f2ee 33d2 xor edx,edx
35e4f2f0 f7c504000000 test ebp,0x4
35e4f2f6 7422 jz 35e4f31a
35e4f2f8 6a00 push 0x0
35e4f2fa 89e3 mov ebx,esp
35e4f2fc ba02000000 mov edx,0x2
35e4f301 b906000000 mov ecx,0x6
35e4f306 8b4304 mov eax,[ebx+0x4]
35e4f309 8903 mov [ebx],eax
35e4f30b 83c304 add ebx,0x4
35e4f30e 49 dec ecx
35e4f30f 75f5 jnz 35e4f306
35e4f311 c70378563412 mov dword ptr [ebx],0x12345678
35e4f317 83ed04 sub ebp,0x4
35e4f31a ff75f4 push dword ptr [ebp-0xc]
35e4f31d 8955f4 mov [ebp-0xc],edx
35e4f320 83ec04 sub esp,0x4
35e4f323 8b45fc mov eax,[ebp-0x4]
35e4f326 8b4d0c mov ecx,[ebp+0xc]
35e4f329 e906000000 jmp 35e4f334

It is clear that ebp is not frame pointer.
The data is also interesting:
0:000> dd 0x0021e83c
0021e83c 0021e84c 1483e675 4085c0a5 0021e86c
0021e84c 35e448b5 40878db1 14831309 12345678

0:000> dd 0021e86c
0021e86c 0021e8a8 35e23c4a 00000000 0000000

0:000> dd 0021e8a8
0021e8a8 0021e900 5b1675f0 35e2bd60 1483e675

0:000> dd 0021e900
0021e900 0021e934 5b1666e4 0021e9a4 00000000

except the 0021e84c is a little bit off off, the rest looks like a linked list.

@derekbruening
Copy link
Contributor Author

From [email protected] on June 26, 2014 15:06:25

Similar errors at http://build.chromium.org/p/chromium.fyi/builders/Windows%20Browser%20%28DrMemory%20full%29%20%2812%29/builds/222 UnloadTest.CrossSiteInfiniteBeforeUnloadSync

@derekbruening
Copy link
Contributor Author

From [email protected] on June 27, 2014 02:25:25

It is clear that ebp is not frame pointer.

How so? ebp is very much the frame pointer. Its value is 0021e848, where you can see the address of the next frame (0021e86c). So the apparent "linked list" is in fact the stack, with each stack frame pointing to the next. Note that the access that DrM complains about is [ebp-0xc], not [ebp].

The C++ code that generated this asm code is LCodeGen::GenerateOsrPrologue, see https://code.google.com/p/v8/source/browse/branches/bleeding_edge/src/ia32/lithium-codegen-ia32.cc#352 . It hasn't changed in 9 months, for whatever that's worth.

What this code does is it moves the entire stack frame by one pointer in the direction of lower addresses in order to 8-byte align it. The 0x12345678 you see at address 0021e858 is the filler value that was put in. Does DrM make any assumptions that stack frames never move?

I don't see what the bug/problem is here. Doesn't the fact that "dd 0x0021e83c" has given you useful results prove that this address is perfectly addressable? I don't see why [frame pointer - 0xc] would be an invalid access. Unless we're actually three pointer sizes away from a stack overflow, but that seems unlikely. What does esp point to?

If the stack frame is empty (no local variables), then it might be that esp == 0021e838. The value 0x0021e84c at address 0x0021e83c would then be a leftover frame pointer from a previous call, which seems plausible. Does DrM complain when addresses just below esp are accessed (i.e. [esp - x] for small x)?

@derekbruening
Copy link
Contributor Author

From [email protected] on June 27, 2014 08:42:11

Does DrM complain when addresses just below esp are accessed (i.e. [esp - x] for small x)?

Yes, any access beyond the top of the stack is considered unaddressable. This type of code is unsafe on 32-bit *nix and frowned upon elsewhere.

@derekbruening
Copy link
Contributor Author

From [email protected] on June 27, 2014 08:48:46

I'm not sure why this was filed in the DrMem tracker. The v8 code violates the safe programming model checked by Dr. Memory, so this seems to be a true positive correctly identified, with the tool working as designed.

@derekbruening
Copy link
Contributor Author

From [email protected] on June 27, 2014 08:54:27

xref issue #1584

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant