Add `AddressSanitizer` to `DEBUG` builds by JackPGreen · Pull Request #1279 · hazelcast/hazelcast-cpp-client

JackPGreen · 2025-05-15T23:03:02Z

#1276 addressed a Segementation fault, but from the error alone, the root cause was not obvious:

31168 Segmentation fault: 11  build/cpp_client

By adding asan to the DEBUG builds, the CI job would've failed with a much more helpful error message:

=================================================================
==37978==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000040 (pc 0x7eff21446d48 bp 0x7efeff67fce0 sp 0x7efeff67fcd0 T22)
==37978==The signal is caused by a READ memory access.
==37978==Hint: address points to the zero page.

    #0 0x7eff21446d48 in hazelcast::logger::enabled(hazelcast::logger::level) /home/runner/work/hazelcast-cpp-client/hazelcast-cpp-client/hazelcast/src/hazelcast/logger.cpp:71
    #1 0x7eff20a05b1d in hazelcast::client::protocol::codec::client_addclusterviewlistener_handler::handle(hazelcast::client::protocol::ClientMessage&) /home/runner/work/hazelcast-cpp-client/hazelcast-cpp-client/hazelcast/generated-sources/src/hazelcast/client/protocol/codec/codecs.cpp:145

{...}

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /home/runner/work/hazelcast-cpp-client/hazelcast-cpp-client/hazelcast/src/hazelcast/logger.cpp:71 in hazelcast::logger::enabled(hazelcast::logger::level)

#1276 addressed a `Segementation fault`, but from the error alone, the root cause was not obvious: ``` 31168 Segmentation fault: 11 build/cpp_client ``` By adding [`asan`](https://learn.microsoft.com/en-us/cpp/sanitizers/asan) to the `DEBUG` builds, the CI job would've failed with a much more helpful error message: ``` ================================================================= ==37978==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000040 (pc 0x7eff21446d48 bp 0x7efeff67fce0 sp 0x7efeff67fcd0 T22) ==37978==The signal is caused by a READ memory access. ==37978==Hint: address points to the zero page. #0 0x7eff21446d48 in hazelcast::logger::enabled(hazelcast::logger::level) /home/runner/work/hazelcast-cpp-client/hazelcast-cpp-client/hazelcast/src/hazelcast/logger.cpp:71 #1 0x7eff20a05b1d in hazelcast::client::protocol::codec::client_addclusterviewlistener_handler::handle(hazelcast::client::protocol::ClientMessage&) /home/runner/work/hazelcast-cpp-client/hazelcast-cpp-client/hazelcast/generated-sources/src/hazelcast/client/protocol/codec/codecs.cpp:145 {...} AddressSanitizer can not provide additional info. SUMMARY: AddressSanitizer: SEGV /home/runner/work/hazelcast-cpp-client/hazelcast-cpp-client/hazelcast/src/hazelcast/logger.cpp:71 in hazelcast::logger::enabled(hazelcast::logger::level) ``` An _alternative_ implementation would've been to hardcode the flags in the `build-unix`/`build-windows` instead.

Co-authored-by: ihsan demir <ihsandemir@gmail.com>

JackPGreen · 2025-05-16T22:27:17Z

@ihsandemir thanks for your feedback. I was actually waiting for a green PR builder before un-drafting this PR but it seems GitHub gave you a heads-up.

The good news is that asan has already spotted some issues:

==37969==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 48 byte(s) in 2 object(s) allocated from:
    #0 0x7fda668fe548 in operator new(unsigned long) ../../../../src/libsanitizer/asan/asan_new_delete.cpp:95
    #1 0x5564bcfc6901 in hazelcast::client::test::thread_pool::ThreadPoolTest_testEqualThreadAndJobs_Test::TestBody() /home/runner/work/hazelcast-cpp-client/hazelcast-cpp-client/hazelcast/test/src/HazelcastTests8.cpp:2733
    #2 0x5564be3078f3 in void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) /home/runner/work/hazelcast-cpp-client/hazelcast-cpp-client/build/hazelcast/test/googletest-src/googletest/src/gtest.cc:2433
    #3 0x5564be2f8fa0 in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) /home/runner/work/hazelcast-cpp-client/hazelcast-cpp-client/build/hazelcast/test/googletest-src/googletest/src/gtest.cc:2469
    #4 0x5564be2a3afd in testing::Test::Run() /home/runner/work/hazelcast-cpp-client/hazelcast-cpp-client/build/hazelcast/test/googletest-src/googletest/src/gtest.cc:2508
    #5 0x5564be2a4fd0 in testing::TestInfo::Run() /home/runner/work/hazelcast-cpp-client/hazelcast-cpp-client/build/hazelcast/test/googletest-src/googletest/src/gtest.cc:2684
    #6 0x5564be2a5d2b in testing::TestSuite::Run() /home/runner/work/hazelcast-cpp-client/hazelcast-cpp-client/build/hazelcast/test/googletest-src/googletest/src/gtest.cc:2816
    #7 0x5564be2c28ef in testing::internal::UnitTestImpl::RunAllTests() /home/runner/work/hazelcast-cpp-client/hazelcast-cpp-client/build/hazelcast/test/googletest-src/googletest/src/gtest.cc:5338
    #8 0x5564be30aca8 in bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) /home/runner/work/hazelcast-cpp-client/hazelcast-cpp-client/build/hazelcast/test/googletest-src/googletest/src/gtest.cc:2433
    #9 0x5564be2fb991 in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) /home/runner/work/hazelcast-cpp-client/hazelcast-cpp-client/build/hazelcast/test/googletest-src/googletest/src/gtest.cc:2469
    #10 0x5564be2bf3ac in testing::UnitTest::Run() /home/runner/work/hazelcast-cpp-client/hazelcast-cpp-client/build/hazelcast/test/googletest-src/googletest/src/gtest.cc:4925
    #11 0x5564be3291f0 in RUN_ALL_TESTS() /home/runner/work/hazelcast-cpp-client/hazelcast-cpp-client/build/hazelcast/test/googletest-src/googletest/include/gtest/gtest.h:2473
    #12 0x5564be32913c in main /home/runner/work/hazelcast-cpp-client/hazelcast-cpp-client/build/hazelcast/test/googletest-src/googletest/src/gtest_main.cc:45
    #13 0x7fda6602a1c9 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
    #14 0x7fda6602a28a in __libc_start_main_impl ../csu/libc-start.c:360
    #15 0x5564bc4f0e14 in _start (/home/runner/work/hazelcast-cpp-client/hazelcast-cpp-client/build/hazelcast/test/src/client_test+0x3e2e14) (BuildId: 1ae11315358a2aa750dab7fc7ac35241e01a2005

...the bad news is that these failures are actually blocking this PR from being merged.

What do you think the next steps should be?

fix the leaks separately (^ that ^ one looks like it's in test code, there are probably others)
try suppressing leaks checking and aim for stacktraces only

The change in hazelcast#1279 made it obvious that adding configuration for `Debug` builds (e.g. "add an extra compilation argument") was very tedious as it needs to be declared in _each_ workflow and then passed all the way down to the actual scripts that build the code. Instead, it'd be much cleaner if the jobs _only_ declare a release type, and the build scripts react to that with the appropriate actions (e.g. adding some extra debug or whatever). Changes: - migrate the _implementation_ of the `DEBUG` builds into the `build-` scripts - e.g. `WARN_AS_ERROR` can be assumed when `DEBUG` is enabled, rather than being duplicated in each job configuration - remove the architecture to pass these redundant params down - pass `BUILD_TYPE` into the `build-` scripts as an environment variable, as used for other parameters - rename `BUILD_CONFIGURATION` to `BUILD_TYPE` in Windows scripts for consistency - enable `WARN_AS_ERROR` on Windows for consistency This PR also helps to make hazelcast#1279 smaller.

ihsandemir · 2025-05-20T10:13:02Z

separately

Test leaks may not be significant, still can be fixed later on. I suggest this:

Have asan work but not fail the build.
Open an issue in the repo to fix it and change the setting so that asan can fail the build.

This may be planned in tech debt for C++ client.

I do not think we need to block any release if these are just test objects related at the moment.

ihsandemir · 2025-05-20T10:16:32Z

+if "%BUILD_CONFIGURATION%"=="Debug" (
+    REM Enable address sanitizer to provide meaningful stack traces
+    REM Not enabled due to failure with tooling on GitHub Actions
+    REM https://github.com/actions/runner-images/issues/7739


The issue seems to have a solution. why commented out?

The solution (explicitly install the "correct" tooling) is non-trivial (what is the correct version for this project? Where does it come from?).

I think have this for a build is enough.

We specify the VC version used at here Minimum supported we test against VC 2019.

We specify the VC version used at here Minimum supported we test against VC 2019.

Unfortunately I don't know how to translate ^ that ^ into a fix for Windows, hence showing an example implementation with a TODO for the future.

JackPGreen · 2025-05-20T15:40:31Z

separately

Test leaks may not be significant, still can be fixed later on. I suggest this:

Have asan work but not fail the build.

7f33c50

Open an issue in the repo to fix it and change the setting so that asan can fail the build.

#1282

…n_r2099816171

The change in #1279 made it obvious that adding configuration for `Debug` builds (e.g. "add an extra compilation argument") was very tedious as it needs to be declared in _each_ workflow and then passed all the way down to the actual scripts that build the code. Instead, it'd be much cleaner if the jobs _only_ declare a release type, and the build scripts react to that with the appropriate actions (e.g. adding some extra debug or whatever). Changes: - migrate the _implementation_ of the `DEBUG` builds into the `build-` scripts - e.g. `WARN_AS_ERROR` can be assumed when `DEBUG` is enabled, rather than being duplicated in each job configuration - remove the architecture to pass these redundant params down - pass `BUILD_TYPE` into the `build-` scripts as an environment variable, as already used for other parameters and for the Windows equivalent (`BUILD_CONFIGURATION`) This PR is the refactor-half of #1279, split out separately to make that PR smaller.

The change in hazelcast#1279 made it obvious that adding configuration for `Debug` builds (e.g. "add an extra compilation argument") was very tedious as it needs to be declared in _each_ workflow and then passed all the way down to the actual scripts that build the code. Instead, it'd be much cleaner if the jobs _only_ declare a release type, and the build scripts react to that with the appropriate actions (e.g. adding some extra debug or whatever). Changes: - migrate the _implementation_ of the `DEBUG` builds into the `build-` scripts - e.g. `WARN_AS_ERROR` can be assumed when `DEBUG` is enabled, rather than being duplicated in each job configuration - remove the architecture to pass these redundant params down - pass `BUILD_TYPE` into the `build-` scripts as an environment variable, as already used for other parameters and for the Windows equivalent (`BUILD_CONFIGURATION`) This PR is the refactor-half of hazelcast#1279, split out separately to make that PR smaller.

The change in #1279 made it obvious that adding configuration for `Debug` builds (e.g. "add an extra compilation argument") was very tedious as it needs to be declared in _each_ workflow and then passed all the way down to the actual scripts that build the code. Instead, it'd be much cleaner if the jobs _only_ declare a release type, and the build scripts react to that with the appropriate actions (e.g. adding some extra debug or whatever). Changes: - migrate the _implementation_ of the `DEBUG` builds into the `build-` scripts - e.g. `WARN_AS_ERROR` can be assumed when `DEBUG` is enabled, rather than being duplicated in each job configuration - remove the architecture to pass these redundant params down - pass `BUILD_TYPE` into the `build-` scripts as an environment variable, as already used for other parameters and for the Windows equivalent (`BUILD_CONFIGURATION`) This PR is the refactor-half of #1279, split out separately to make that PR smaller.

JackPGreen · 2025-06-09T10:40:34Z

@ihsandemir are you able to review, please?

JackPGreen · 2025-06-16T09:31:29Z

@ihsandemir are you able to review, please?

@ihsandemir bump :)

Co-authored-by: ihsan demir <ihsandemir@gmail.com>

JackPGreen self-assigned this May 15, 2025

ihsandemir requested changes May 16, 2025

View reviewed changes

Comment thread scripts/build-unix.sh Outdated

Comment thread scripts/build-unix.sh Outdated

Comment thread scripts/build-windows.bat Outdated

Comment thread scripts/build-windows.bat Outdated

JackPGreen and others added 4 commits May 16, 2025 22:39

Remove case insensitivity

6d44615

Update scripts/build-unix.sh

401d622

Co-authored-by: ihsan demir <ihsandemir@gmail.com>

Apply suggestions from code review

5c0b435

Co-authored-by: ihsan demir <ihsandemir@gmail.com>

Fix matrix names

e5eb329

JackPGreen requested a review from ihsandemir May 16, 2025 22:27

JackPGreen marked this pull request as ready for review May 16, 2025 22:27

JackPGreen force-pushed the add-`AddressSanitizer`-3 branch from b7cef7f to 573b430 Compare May 17, 2025 09:05

JackPGreen mentioned this pull request May 17, 2025

Simplify Debug/Release options #1280

Merged

JackPGreen force-pushed the add-`AddressSanitizer`-3 branch 3 times, most recently from 7dfca18 to 7d23115 Compare May 17, 2025 20:50

Remove for Windows

e1a6f1e

JackPGreen force-pushed the add-`AddressSanitizer`-3 branch from 7d23115 to e1a6f1e Compare May 17, 2025 20:51

ihsandemir requested changes May 20, 2025

View reviewed changes

JackPGreen mentioned this pull request May 20, 2025

Memory Leak in tests [API-2347] #1282

Open

JackPGreen added 2 commits May 20, 2025 16:37

Disable leak detection

7f33c50

Windows comment consistency

7ad4b8b

JackPGreen force-pushed the add-`AddressSanitizer`-3 branch from 67b7026 to 7ad4b8b Compare May 20, 2025 15:38

JackPGreen requested a review from ihsandemir May 20, 2025 15:40

ihsandemir reviewed May 21, 2025

View reviewed changes

Comment thread scripts/test-unix.sh Outdated

https://github.com/hazelcast/hazelcast-cpp-client/pull/1279#discussio…

4ab2a53

…n_r2099816171

JackPGreen requested a review from ihsandemir May 21, 2025 09:38

Merge branch 'master' into add-AddressSanitizer-3

43d7268

JackPGreen enabled auto-merge (squash) May 21, 2025 09:54

Fix coverage

826d2b3

ihsandemir reviewed May 26, 2025

View reviewed changes

Comment thread scripts/test-windows.bat Outdated

JackPGreen requested a review from ihsandemir May 26, 2025 15:48

Update scripts/test-windows.bat

dcfa13d

Co-authored-by: ihsan demir <ihsandemir@gmail.com>

ihsandemir approved these changes Jun 16, 2025

View reviewed changes

JackPGreen disabled auto-merge June 23, 2025 08:40

Merge branch 'master' into add-AddressSanitizer-3

2e839ed

JackPGreen enabled auto-merge (squash) June 23, 2025 08:40

JackPGreen merged commit 68f7f56 into master Jun 23, 2025
90 of 91 checks passed

JackPGreen deleted the add-`AddressSanitizer`-3 branch June 23, 2025 09:33

Uh oh!

Conversation

JackPGreen commented May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JackPGreen commented May 16, 2025

Uh oh!

ihsandemir commented May 20, 2025

Uh oh!

ihsandemir May 20, 2025

Choose a reason for hiding this comment

Uh oh!

JackPGreen May 20, 2025

Choose a reason for hiding this comment

Uh oh!

ihsandemir May 20, 2025

Choose a reason for hiding this comment

Uh oh!

JackPGreen May 20, 2025

Choose a reason for hiding this comment

Uh oh!

JackPGreen commented May 20, 2025

Uh oh!

Uh oh!

Uh oh!

JackPGreen commented Jun 9, 2025

Uh oh!

JackPGreen commented Jun 16, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

JackPGreen commented May 15, 2025 •

edited

Loading