Skip to content

Drop the ecdsa_preprocessing_data destructor (UB + heap leak)#54

Open
kozyilmaz wants to merge 1 commit into
fireblocks:mainfrom
kozyilmaz:ecdsa-preproc-leak-fix
Open

Drop the ecdsa_preprocessing_data destructor (UB + heap leak)#54
kozyilmaz wants to merge 1 commit into
fireblocks:mainfrom
kozyilmaz:ecdsa-preproc-leak-fix

Conversation

@kozyilmaz

Copy link
Copy Markdown
Contributor

It cleansed sizeof(struct) from k.data (== this), zeroing the vector/map control blocks before their destructors ran -- UB, leaking the buffers. The scalars already self-wipe and the containers are public, so it's unneeded.

This has been brought up by @nkavian, I kept the proving tests in a separate branch not to pollute your codebase. The test that proves the leak/UB lives in this branch kozyilmaz@5cb0aac

The fix is tested on the system below:

$ lsb_release -a
No LSB modules are available.
Distributor ID:	Ubuntu
Description:	Ubuntu 26.04 LTS
Release:	26.04
Codename:	resolute

$ uname -a
Linux utgard 7.0.0-22-generic #22-Ubuntu SMP PREEMPT_DYNAMIC Mon May 25 15:37:49 UTC 2026 aarch64 GNU/Linux

This is the test and output in the other branch (kozyilmaz@5cb0aac):

**Configure (one-time):**
cmake -S . -B build
**Build — just the leak test (fast):**
cmake --build build --target cosigner_preprocessing_data_leak_test -j$(nproc)
**Run — the leak test directly (clearest output):**
./build/test/cosigner/cosigner_preprocessing_data_leak_test

$ ./build/test/cosigner/cosigner_preprocessing_data_leak_test

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cosigner_preprocessing_data_leak_test is a Catch v2.13.8 host application.
Run with -? for options

-------------------------------------------------------------------------------
ecdsa_preprocessing_data destructor leaks its std::vector/std::map members
-------------------------------------------------------------------------------
/home/loki/devel/mpc-lib/test/cosigner/ecdsa_preprocessing_data_leak_test.cpp:53
...............................................................................

/home/loki/devel/mpc-lib/test/cosigner/ecdsa_preprocessing_data_leak_test.cpp:69: FAILED:
  REQUIRE( live == 0 )
with expansion:
  5 == 0
with message:
  heap allocations still live after destruction: 5 (a correct destructor frees
  them all, leaving 0)

===============================================================================
test cases: 1 | 1 failed
assertions: 1 | 1 failed

After this one line fix the test is cleared:

$ ./build/test/cosigner/cosigner_preprocessing_data_leak_test
===============================================================================
All tests passed (1 assertion in 1 test case)

It cleansed sizeof(struct) from k.data (== this), zeroing the vector/map control blocks before their destructors ran -- UB, leaking the buffers. The scalars already self-wipe and the containers are public, so it's unneeded.

Signed-off-by: kozyilmaz <kazim@monolytic.com>
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.

1 participant