Skip to content

Conversation

@kevinAlbs
Copy link
Contributor

@kevinAlbs kevinAlbs commented Dec 19, 2024

Summary

  • Fix Evergreen config to apply -fsanitize=address.
  • Suppress dlerror leak. Fix test leaks.
  • Restore default of ENABLE_EXTRA_ALIGNMENT=OFF to imported libbson.

Verified by this patch build.

As a drive-by fix, the unused build-and-test-asan-s390x is removed.

Background

#622 accidentally disabled ASan due to a bash comment (# Add ...) appearing in a folded (>-) YAML block:

compile_env: >-
    ${compile_env|}
    LIBMONGOCRYPT_EXTRA_CFLAGS="-fsanitize=address -pthread"
    # Add detect_odr_violation=0 to ASAN_OPTIONS to ignore odr-violation in IntelDFP symbol: __dpml_bid_globals_table
    ASAN_OPTIONS="detect_leaks=1 detect_odr_violation=0"

This PR moves the comment outside of the block to fix:

# Add detect_odr_violation=0 to ASAN_OPTIONS to ignore odr-violation in IntelDFP symbol: __dpml_bid_globals_table
compile_env: >-
    ${compile_env|}
    LIBMONGOCRYPT_EXTRA_CFLAGS="-fsanitize=address -pthread"
    ASAN_OPTIONS="detect_leaks=1 detect_odr_violation=0"

Re-enabling ASan revealed an Indirect leak reported in dlerror only on Ubuntu 22.04 arm64. The leak is now suppressed. I expect it is not an issue in libmongocrypt.

Re-enabling ASan revealed a variety of errors: stack-use-after-scope, stack-buffer-overflow, and a stack-use-after-scope. All were fixed by restoring a default of ENABLE_EXTRA_ALIGNMENT=OFF when importing libbson. Prints were added to pkgconfig-tests.sh and linker-tests.sh to help identify a failing tests.

Restoring ENABLE_EXTRA_ALIGNMENT=OFF default

#875 accidentally changed the default applied ENABLE_EXTRA_ALIGNMENT from false (via empty string) to true (via libbson default).

Restoring the default of ENABLE_EXTRA_ALIGNMENT=OFF triggered a crash in pkgconfig-tests.sh only on RHEL 8.1 ppc64el. The crash occurred in the libmongocrypt dynamic linking against libbson cases. I expect this is caused by libmongocrypt building against the imported libbson with extra alignment disabled. Then using the installed shared libbson with extra alignment enabled. I am not sure why this crash did not occur before #875. A patch build on the commit before #875 passes, but crashes after upgrading to libbson from 1.27.1 to 1.28.1 (current version used by libmongocrypt). I expect the test previously passed "by accident". The test is updated to ensure matching alignment.

Remove the comment from the YAML folded line to fix applying environment variables.
To ignore a "Indirect leak" reported on Ubuntu 22.04 arm64. Assumed not to be an issue in libmongocrypt.
Fixes observed errors on older ASan with older gcc.
To match installed libbson. Fixes crash on RHEL 8.1 ppc64el.
To help identify a failing test.
To help identify a failing test.
This may impact downstream consumers. Example: build with imported libbson, then load a different libbson at runtime. This was the case in the pkgconfig tests. Disabling alignment was the default in 1.11 and accidentally changed in 1.12.0.
@kevinAlbs kevinAlbs force-pushed the rebased.fix-asan.M731 branch from 37c6f7f to 1f96cc4 Compare December 20, 2024 13:45
@kevinAlbs kevinAlbs marked this pull request as ready for review December 20, 2024 13:47
Copy link
Contributor

@eramongodb eramongodb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor suggestions; otherwise, LGTM.

# Enable extra alignment on imported libbson to match installed libbson.
run_cmake -DUSE_SHARED_LIBBSON=ON \
-DENABLE_BUILD_FOR_PPA=OFF \
-DENABLE_EXTRA_ALIGNMENT=ON \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have more information about the "installed libbson" library? Can it be updated so it is also configured with ENABLE_EXTRA_ALIGNMENT=OFF?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes the "installed libbson" is installed within this script. Updated the "installed libbson" to set ENABLE_EXTRA_ALIGNMENT=OFF to match recommended libbson default.

kevinAlbs and others added 3 commits December 30, 2024 15:21
Co-authored-by: Ezra Chung <88335979+eramongodb@users.noreply.github.com>
Matches recommended default in libbson.
@kevinAlbs kevinAlbs requested review from rcsanchez97 and removed request for vector-of-bool December 30, 2024 20:30
The `macos-1100` distro appears to have a system install of libbson with extra alignment enabled. libmongocrypt detects the system install and defaults to enable extra alignment to try to agree. This commit sets  `ENABLE_EXTRA_ALIGNMENT=OFF` for both libbson and libmongocrypt to ensure agreement.
To match libbson recommendeded defaults.
Copy link
Contributor

@rcsanchez97 rcsanchez97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kevinAlbs kevinAlbs merged commit e5181e9 into mongodb:master Jan 2, 2025
50 of 53 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants