Skip to content

Fix memory leak when creating MPIs for ECGroup - #5381

Merged
randombit merged 1 commit into
randombit:masterfrom
arckoor:python-ec-group-memleak
Feb 22, 2026
Merged

Fix memory leak when creating MPIs for ECGroup#5381
randombit merged 1 commit into
randombit:masterfrom
arckoor:python-ec-group-memleak

Conversation

@arckoor

@arckoor arckoor commented Feb 22, 2026

Copy link
Copy Markdown
Contributor

In #5166 (comment) copilot called out that what I was doing in the python code would leak the MPI initialized by __init__. This is the same thing ECGroup is already doing, so fix it.
To be honest, I don't love this fix, I've omitted the | c_void_p from the MPILike type hint, because it's used in other places and I don't think encouraging people to pass a magic pointer that hopefully is an MPI to something like PrivateKey.load_ecdsa(curve: str, x: MPILike) is a good idea.
The MPI class is very unfortunate, all the other classes basically just assign a pointer to self.__obj and let whatever else do the proper initialization, MPI is seemingly the only one that does extra stuff in its constructor.
I considered just skipping __init__ and implementing / calling __new__ directly, but it appears that is frowned upon in the python community (which is probably a good thing).

@arckoor
arckoor force-pushed the python-ec-group-memleak branch from d45efc6 to a92e237 Compare February 22, 2026 15:16
@randombit
randombit requested a review from Copilot February 22, 2026 15:40

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This pull request fixes memory leaks in the ECGroup class methods that retrieve MPI (multi-precision integer) values. The issue was that calling MPI() initializes an MPI object by allocating memory via botan_mp_init, but then the FFI functions like botan_ec_group_get_p would overwrite the pointer with a newly allocated MPI, causing the first allocation to leak.

Changes:

  • Modified MPI.init to accept c_void_p and skip initialization when a raw pointer is provided
  • Updated all six ECGroup getter methods (get_p, get_a, get_b, get_g_x, get_g_y, get_order) to use the new pattern
  • Updated version comment from 3.10.0 to 3.11.0

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@coveralls

Copy link
Copy Markdown

Coverage Status

coverage: 90.332% (-0.003%) from 90.335%
when pulling a92e237 on arckoor:python-ec-group-memleak
into ef0dcf4 on randombit:master.

@randombit
randombit merged commit 5166623 into randombit:master Feb 22, 2026
50 checks passed
@arckoor
arckoor deleted the python-ec-group-memleak branch February 22, 2026 16:07
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.

4 participants