1751823 - gecko/mozglue/tests/gtest/TestStackWalk.cpp:138:5: runtime error: call to function StackWalkTester::LeafCallback(int, int, int, StackWalkTester&) through pointer to incorrect function type 'int (*)(int, int, int, StackWalkTester &)'

Status: RESOLVED FIXED

(Core :: mozglue, defect)

Description

[Tyson Smith [:tsmith]]

This was found by enabling the function check in UBSan and running existing tests. This type of issue can create inconsistencies across platforms, architectures and optimization levels.

Found with m-c 20220124-9b23d1bb84b.

To enable this check add the following to your mozconfig:

ac_add_options --enable-undefined-sanitizer="function"

This issue is found by the existing gtest: TestStackWalk.StackWalk

INFO -  TEST-START | TestStackWalk.StackWalk
INFO -  /builds/worker/checkouts/gecko/mozglue/tests/gtest/TestStackWalk.cpp:138:5: runtime error: call to function StackWalkTester::LeafCallback(int, int, int, StackWalkTester&) through pointer to incorrect function type 'int (*)(int, int, int, StackWalkTester &)'
INFO -  /builds/worker/checkouts/gecko/mozglue/tests/gtest/TestStackWalk.cpp:144: note: StackWalkTester::LeafCallback(int, int, int, StackWalkTester&) defined here
INFO -      #0 0x7f3e3806d577 in int StackWalkTester::IntermediateCallback<3>(int, int, int, StackWalkTester&) /builds/worker/checkouts/gecko/mozglue/tests/gtest/TestStackWalk.cpp:138:5
INFO -      #1 0x7f3e3806d33f in int StackWalkTester::IntermediateCallback<2>(int, int, int, StackWalkTester&) /builds/worker/checkouts/gecko/mozglue/tests/gtest/TestStackWalk.cpp:138:5
INFO -      #2 0x7f3e3806d17f in int StackWalkTester::IntermediateCallback<1>(int, int, int, StackWalkTester&) /builds/worker/checkouts/gecko/mozglue/tests/gtest/TestStackWalk.cpp:138:5
INFO -      #3 0x7f3e3806da54 in StackWalkTester::RunTest(int) /builds/worker/checkouts/gecko/mozglue/tests/gtest/TestStackWalk.cpp:233:5
INFO -      #4 0x7f3e3806cf3a in TestStackWalk_StackWalk_Test::TestBody() /builds/worker/checkouts/gecko/mozglue/tests/gtest/TestStackWalk.cpp:270:14
INFO -      #5 0x7f3e38dbcb9c in testing::Test::Run() /builds/worker/checkouts/gecko/testing/gtest/gtest/src/gtest.cc:2519:5
INFO -      #6 0x7f3e38dbed9d in testing::TestInfo::Run() /builds/worker/checkouts/gecko/testing/gtest/gtest/src/gtest.cc:2695:11
INFO -      #7 0x7f3e38dc0253 in testing::TestCase::Run() /builds/worker/checkouts/gecko/testing/gtest/gtest/src/gtest.cc:2813:28
INFO -      #8 0x7f3e38dd7293 in testing::internal::UnitTestImpl::RunAllTests() /builds/worker/checkouts/gecko/testing/gtest/gtest/src/gtest.cc:5179:43
INFO -      #9 0x7f3e38dd6869 in testing::UnitTest::Run() /builds/worker/checkouts/gecko/testing/gtest/gtest/src/gtest.cc:4788:10
INFO -      #10 0x7f3e38e00130 in RUN_ALL_TESTS /builds/worker/workspace/obj-build/dist/include/gtest/gtest.h:2342:46
INFO -      #11 0x7f3e38e00130 in mozilla::RunGTestFunc(int*, char**) /builds/worker/checkouts/gecko/testing/gtest/mozilla/GTestRunner.cpp:156:10
INFO -      #12 0x7f3e4603e996 in XREMain::XRE_mainStartup(bool*) /builds/worker/checkouts/gecko/toolkit/xre/nsAppRunner.cpp:4394:16
INFO -      #13 0x7f3e4604cc36 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /builds/worker/checkouts/gecko/toolkit/xre/nsAppRunner.cpp:5527:12
INFO -      #14 0x7f3e4604da8b in XRE_main(int, char**, mozilla::BootstrapConfig const&) /builds/worker/checkouts/gecko/toolkit/xre/nsAppRunner.cpp:5598:21
INFO -      #15 0x565203acf962 in do_main(int, char**, char**) /builds/worker/checkouts/gecko/browser/app/nsBrowserApp.cpp:225:22
INFO -      #16 0x565203acebae in main /builds/worker/checkouts/gecko/browser/app/nsBrowserApp.cpp:395:16
INFO -      #17 0x7f3e5f73cb96 in __libc_start_main /tmp/glibc/csu/../csu/libc-start.c:310
INFO -      #18 0x565203a1d80c in _start (/builds/worker/workspace/build/application/firefox/firefox+0x6f80c)

Comment 1

[Mike Hommey [:glandium]]

That's done on purpose to trigger a tail-call optimization on the call to MozStackWalk. We don't actually care about the returned value, but we do care about there being a return value for the intermediate frame callbacks. It's all test code anyways. How do we add an exception for this?

Comment 2

[Tyson Smith [:tsmith]]

We can use __attribute__((no_sanitize("undefined"))) or even better __attribute__((no_sanitize("function"))). This has the benefit of making it clear that this is intentional but it might not be supported by other compilers. If this is not an option let me know and I will add it to a suppression list.

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:glandium, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 4

[Tyson Smith [:tsmith]]

Glandium, would something like this be acceptable?

-  MOZ_NEVER_INLINE MOZ_EXPORT static void LeafCallback(
-      int aDepth, int aLastSkipped, int aIgnored, StackWalkTester& aTester) {
+  __attribute__((no_sanitize("function")))
+  MOZ_NEVER_INLINE MOZ_EXPORT static void
+  LeafCallback(int aDepth, int aLastSkipped, int aIgnored,
+               StackWalkTester& aTester) {
     if (aDepth == aLastSkipped) {
       aTester.mFirstFramePC = CallerPC();
     }

Would that be compatible with all supported compilers or is there anything obviously wrong?

Comment 5

[Tyson Smith [:tsmith]]

Attachment: Bug 1751823 - Add UBSan suppression to test. r=glandium

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/86152795bd08
Add UBSan suppression to test. r=glandium

Comment 7

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/86152795bd08