Changes regarding (debug) symbols in 3.8.x?

[tobiasbrunner]

Building against 3.8.1 required enabling backtraces via libbfd to whitelist functions in our own leak detective (instead of just using dladdr(), which worked before).

A second thing I had to change was whitelisting botan_privkey_load.cold (in addition to botan_privkey_load for older versions) even though I don't see the cold attribute getting used anywhere in the Botan code.

Can you point me to the changes between 3.7.1 and 3.8.1 that caused either or both of these things?

[randombit]

Can you point me to the changes between 3.7.1 and 3.8.1 that caused either or both of these things?

Any change that caused a behavioral difference in this way was not intentional. There were between 3.7.1 and 3.8.1 no change in compiler flags used (with the sole exception relating to LoongArch SIMD instructions, and only used on that architecture). Generating .cold functions seems to be under the control of GCC's option -freorder-blocks-and-partition which (per GCC info pages) is enabled at -O2 and higher on x86. And botan_privkey_load itself had no changes in 3.7.1..3.8.1

In comparing the behavior of 3.7.1 vs 3.8.1 are you using the same version of GCC, same options, etc?

[tobiasbrunner]

Thanks for looking into it.

In comparing the behavior of 3.7.1 vs 3.8.1 are you using the same version of GCC, same options, etc?

I think so. Here is a failing build with 3.8.1 that used image 20250427.1.0, where you can see the missing symbols. And here is a test build I ran with 3.7.1 that uses the same image, but here the symbols were still resolved without compiling with --enable-bfd-backtraces, and there was no .cold suffix, so it succeeded.

[tobiasbrunner]

@randombit By the way, for a more direct comparison, here is a successful build with 3.7.1, and here a failing build with 3.8.1 with only two additional commits, one changes botan_privkey|pubkey_export to corresponding view functions (to avoid deprecation warnings) and the other is the version bump to 3.8.1. The build image was the same as above.

Note that in the "latest (all, clang, yes, no)" case Botan is still built with GCC, only strongSwan is built with clang. The build log for Botan shows the same GCC version: success, failure (some differences can be seen, e.g. 3.7.1 reporting Assuming target x86_64 is little endian, or 3.8.1 disabling sha2_64_x86, but these are probably intended changes). Also, this "all"-build has --enable-bfd-backtraces enabled, so the non-whitelisted symbol with .cold suffix can be seen in the failing build's log.

Compared to that, in the "crypto-plugins (ubuntu-latest, botan, yes)" case, the above option is disabled, so symbols are not resolved in the failing case and whitelisting didn't work either (again, the GCC version is the same for the successful and failing runs).

[tobiasbrunner]

@randombit Any updates?

[tobiasbrunner]

I've tested 3.9.0 and I see the same issues (here without BFD-based backtraces, then with but without .cold symbol whitelisted, then successful after adding both workarounds).

Do you think it's possible to determine the cause somehow? It's really weird that this just started to happen without changes in compiler flags (same compiler, different codebase, i.e. just 3.7.x -> 3.8.x+).

We also have a Debian-based regression test environment where we have several scenarios that use Botan. So far I haven't seen these issues there (neither with bookworm and GCC 12.2, nor trixie and GCC 14.2). It only happens in these GitHub Action images with GCC 13.3. The configure options are basically the same, the only difference is that we build with --amalgamation on GitHub Actions (this reduced the build time by almost 50% back in 2018, not sure if that would still be the case nowadays). But I tried disabling it and it didn't make a difference.

[reneme]

I managed to reproduce this in the GitHub Actions in our fork of strongswan as well as using a docker container (see below). Based on the reproducer, git bisect points to 7322df4 as the commit that introduced this.

The detected leak (72 bytes) is an error message from a caught exception, namely "PKCS #8 private key decoding failed with Unknown PKCS #8 version number", that the FFI wrapper stores in a global thread-local variable:

botan/src/lib/ffi/ffi.cpp

Lines 90 to 92 in df91b27

	int ffi_error_exception_thrown(const char* func_name, const char* exn, int rc) {
	g_last_exception_what.assign(exn);

... my best guess: this is the reason why botan_private_key_load was white listed in leak_detective.c in the first place. Probably due to the refactoring in 7322df4 this assignment now shows up somewhere else. For the record: the detected leak is a false-positive, the reference to the memory is not lost and will be deallocated later (e.g. when ffi_clear_last_exception() is called).

Details

Relevant log message of failed GitHub Action

  Running suite 'ecdsa':
    Running case 'generate': +
    Running case 'load': -+++
      Failure in 'test_load': Leak detected: 1 allocations using 72 bytes (i = 0)
 dumping 13 stack frame addresses:
  /lib/x86_64-linux-gnu/libstdc++.so.6 @ 0x7f9439a00000 (_Znwm+0x24) [0x7f9439abb904]
    -> ??:?
  /usr/local/lib/libbotan-3.so.8 @ 0x7f9439e00000 (_ZNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEE9_M_mutateEmmPKcm+0x8f) [0x7f943a21f74f]
    -> ??:?
  /usr/local/lib/libbotan-3.so.8 @ 0x7f9439e00000 (_ZNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEE10_M_replaceEmmPKcm+0xfb) [0x7f943a21f96b]
    -> ??:?
  /usr/local/lib/libbotan-3.so.8 @ 0x7f9439e00000 [0x7f9439faeda4]
    -> :?
  /usr/local/lib/libbotan-3.so.8 @ 0x7f9439e00000 [0x7f9439f0590d]
    -> botan_all.cpp:?
  /home/runner/work/strongswan/strongswan/src/libstrongswan/plugins/botan/.libs/libstrongswan-botan.so @ 0x7f943a77d000 (botan_private_key_load+0x8e) [0x7f943a788c6e]
    -> /home/runner/work/strongswan/strongswan/src/libstrongswan/plugins/botan/botan_util_keys.c:194
  /home/runner/work/strongswan/strongswan/src/libstrongswan/.libs/libstrongswan.so.0 @ 0x7f943a7f5000 [0x7f943a8242e6]
    -> /home/runner/work/strongswan/strongswan/src/libstrongswan/credentials/credential_factory.c:156
  /home/runner/work/strongswan/strongswan/src/libstrongswan/tests/.libs/libstrongswan_tests @ 0x556527428000 [0x556527480fe9]
    -> ??:0
  /home/runner/work/strongswan/strongswan/src/libstrongswan/tests/.libs/libstrongswan_tests @ 0x556527428000 [0x55652749edc0]
    -> ??:0
  /home/runner/work/strongswan/strongswan/src/libstrongswan/tests/.libs/libstrongswan_tests @ 0x556527428000 [0x55652749fe30]
    -> ??:0
  /lib/x86_64-linux-gnu/libc.so.6 @ 0x7f943a400000 [0x7f943a42a1ca]
    -> ??:?
  /lib/x86_64-linux-gnu/libc.so.6 @ 0x7f943a400000 (__libc_start_main+0x8b) [0x7f943a42a28b]
    -> ??:?
  /home/runner/work/strongswan/strongswan/src/libstrongswan/tests/.libs/libstrongswan_tests @ 0x556527428000 [0x556527442245]
    -> ??:0
  Passed 1/2 'ecdsa' test cases

Dockerfile used for local reproduction

Simply run docker build . on this and update the git reference of the Botan checkout accordingly.

FROM ubuntu:24.04

RUN apt update -y && apt upgrade -y
RUN apt install -y \
            autoconf \
            automake \
            binutils-dev \
            bison \
            clearsilver-dev \
            flex \
            git \
            gperf \
            libcurl4-gnutls-dev \
            libfcgi-dev \
            libgcrypt20-dev \
            libgmp-dev \
            libiptc-dev \
            libjson-c-dev \
            libldap2-dev \
            libldns-dev \
            libmysqlclient-dev \
            libnm-dev \
            libpam0g-dev \
            libpcsclite-dev \
            libselinux1-dev \
            libsoup-3.0-dev \
            libsqlite3-dev \
            libsystemd-dev \
            libtool \
            libtspi-dev \
            libunbound-dev \
            pkgconf \
            python3-build \
            ruby-rubygems \
            tox

RUN echo "building botan..." && \
    git clone https://github.com/randombit/botan.git botan && \
    cd botan && \
    git checkout -qf 7322df4ff6cd4e92a8ef85040cb47f58e9dea398 && \
    ./configure.py --without-os-features=threads \
                   --disable-modules=locking_allocator \
                   --disable-modules=pkcs11,tls,x509,xmss \
                   --disable-deprecated-features \
                   --enable-modules=md5 \
                   --prefix=/usr/local \
                   --build-targets=static,shared && \
    make -j$(nproc) libs && \
    make install && \
    ldconfig

RUN echo "testing strongswan..." && \
    git clone https://github.com/strongswan/strongswan.git strongswan && \
    cd strongswan && \
    ./autogen.sh && \
    CC=gcc CFLAGS="-g -O2" ./configure \
        --disable-defaults \
        --enable-pki \
        --enable-botan \
        --enable-pem \
        --enable-hmac \
        --enable-x509 \
        --enable-constraints \
        --enable-drbg \
        --disable-dependency-tracking \
        --enable-silent-rules \
        --enable-test-vectors \
        --enable-monolithic=no \
        --enable-leak-detective=yes \
        --disable-asan && \
    TESTS_SUITES=ecdsa make check

[tobiasbrunner]

The problem isn't the leak as such (allocations like that are often an issue for our leak detective). If we can't get rid of it, ideally, we can whitelist a function close to the actual culprit. That wasn't really possible here so we whitelisted botan_privkey_load (not sure if it makes much of a difference compared to botan_private_key_load, but we call botan_privkey_load directly in some cases anyway).

This symbol doesn't show up in the backtraces anymore now. Instead we get botan_private_key_load.cold (if we get any, see below). We can add that to the whitelist, but the question is why it happens in the first place.

The other issue is that whitelisting either symbol isn't possible anymore at all without libbfd (which is slower than the default that worked previously). You do see the .cold symbol in the "latest (all, ...)" jobs as everything is enabled there, including --enable-bfd-backtraces. If other whitelisted symbols are affected or this is specific to botan_privkey_load, I don't know. Again, we could change that but the question is why this happens. Maybe also due to inlining?

I wonder if we could avoid the leak if we'd call ffi_clear_last_exception() when the main thread unloads the botan plugin, which happens before we collect leaks. It doesn't seem to be a public function, though. Can we clear that error buffer in some other way before the main thread exits?

[reneme]

Again, we could change that but the question is why this happens. Maybe also due to inlining?

Indeed, the failed symbol resolution seems to be due to the additional inlining facilitated by 7322df4. When I revert this by marking ffi_guard_thunk with __attribute__((noinline)), the whitelist entry is effective and the false-positive leak is filtered as expected.

If other whitelisted symbols are affected or this is specific to botan_privkey_load, I don't know.

Given that ffi_guard_thunk is used in virtually all FFI functions, this issue potentially affects almost all of them. I.e. whitelisting Botan's FFI functions in the way your leak detector implements it is probably not effective any longer with Botan 3.8 and newer, at least without --enable-bfd-backtraces. 😞

It doesn't seem to be a public function, though. Can we clear that error buffer in some other way before the main thread exits?

There's no explicit public API that clears the error buffer (as this isn't really a sensible use case, for most users). As a workaround, you could invoke an FFI function that uses the ffi_guard_thunk internally and that succeeds reliably. In the ffi_guard_thunk the error buffer is always cleared before any other error may be detected. (For the record: this behavior isn't guaranteed by the API contract and might change unexpectedly. It would really just be a workaround.)

A good candidate for implementing such a workaround may be botan_hex_encode() with a hardcoded short buffer. Its likely never going to fail and it'll do the trick to the extent explained above.

I hope this helps in finding a better workaround. Feel free to ping me for a code review or further input on this. For the time being, I think this solved from our side.

[tobiasbrunner]

Indeed, the failed symbol resolution seems to be due to the additional inlining facilitated by 7322df4. When I revert this by marking ffi_guard_thunk with __attribute__((noinline)), the whitelist entry is effective and the false-positive leak is filtered as expected.

Thanks for testing. If it becomes an issue, patching that might be a workaround in our testing environment where using --enable-bfd-backtraces isn't ideal due to the overhead. Another workaround is to just whitelist botan_private_key_load instead of botan_privkey_load variants, as that still shows up in the backtrace even without bfd-backtraces and probably doesn't make that much of a difference, and we already whitelist botan_public_key_load (although that actually doesn't seem to be necessary anymore, botan_kdf neither). So that might be the simplest "fix" for now.

A good candidate for implementing such a workaround may be botan_hex_encode() with a hardcoded short buffer. Its likely never going to fail and it'll do the trick to the extent explained above.

Hehe, I landed on the same function in my tests. Unfortunately, it does not get rid of that particular leak. What's interesting about it is that it only shows up as a leak once, the very first time. Subsequent tests that hit the same code paths and trigger the exception don't show up as leak anymore (different error messages that follow don't trigger it again either). My guess is that the buffer that's allocated to store the error message is only allocated once (and larger than the actual error message) and then gets reused for subsequent error messages, and that clearing it doesn't really help as it doesn't free the memory (might just return it to a pool of string buffers or really just set the length to 0). So only the first time an exception occurs does the leak detective actually register an allocation that's not freed.

Another thing I noticed is that there are other "leaks" as well if not whitelisting botan_privkey_create. In particular for EC groups that are apparently cached statically (triggered when generating ECDSA keys). We could whitelist two very specific mangled symbols to get rid of these (_ZN5Botan8EC_Group9from_nameESt17basic_string_viewIcSt11char_traitsIcEE and _ZN5Botan8EC_Group18known_named_groupsB5cxx11Ev), but that's probably neither portable nor stable.

So for now the simplest way forward is probably to just whitelist botan_privkey_create and botan_private_key_load (no others seem to be required with current Botan versions). GHA currently has Ubuntu sync issues, so don't know if it works yet (it works locally and in our testing environment). Will try again tomorrow.

[tobiasbrunner]

So for now the simplest way forward is probably to just whitelist botan_privkey_create and botan_private_key_load (no others seem to be required with current Botan versions).

botan_privkey_load (which we also call directly and then shows up in backtraces) and botan_privkey_load_rsa_pkcs1 are still necessary, not for the libstrongswan unit tests or the testing environment, but the unit tests of libtls, which uses its own sets of RSA/ECDSA credentials that hit different code paths. Now it works with Botan 3.10.0 without bfd-backtraces.

[reneme]

My guess is that the buffer that's allocated to store the error message is only allocated once (and larger than the actual error message) and then gets reused for subsequent error messages, and that clearing it doesn't really help as it doesn't free the memory (might just return it to a pool of string buffers or really just set the length to 0).

I agree. That's exactly what happens. Looking into the STL documentation of std::string::clear(), it says the C++ standard does not explicitly require that capacity is unchanged by this function, but existing implementations do not change capacity. This means that they do not release the allocated memory. So yes: unfortunately that's not a suitable workaround after all.

In particular for EC groups that are apparently cached statically (triggered when generating ECDSA keys).

That's what happens, yes. Internally, there is a EC_Group_Data_Map that caches previously created ECC groups as global state. See here:

botan/src/lib/pubkey/ec_group/ec_group.cpp

Lines 175 to 184 in 2d2b533

	EC_Group_Data_Map& EC_Group::ec_group_data() {
	/*
	* This exists purely to ensure the allocator is constructed before g_ec_data,
	* which ensures that its destructor runs after ~g_ec_data is complete.
	*/
	static Allocator_Initializer g_init_allocator;
	static EC_Group_Data_Map g_ec_data;
	return g_ec_data;
	}

... whitelisting the mangled symbols is risky at best.