Memory Leaks in Structure and Field

[PyWoody]

There is a memory leak in the way smbprotocol.structure.Structure is initialized as well as memory leaks in the use of lambdas in smbprotocol.structure.Field subclass creations.

My branch at memleakstatic demonstrates ways to resolve both issues. In my testing, memory usage is dramatically reduced and flat across file transfers.

The memory leak with Structure can be avoided by having the reference back to self be a weakref.proxy to not increase its reference count.

The memory leaks associated with how Field subclasses are created requires moving from lambdas in the constructors to @staticmethods in the respective subclasses. By using @staticmethods, the class instance is not captured in the lambda, which would create a cyclical reference preventing garbage collection.

Below contains steps on how to reproduce and test the memory leaks as well as how I was able to trace it to the sources.

I would be glad to make a PR for any changes or to test against any specific edge cases. The most obvious edge-case I can think of would be in any shutdown situation that expects a Structure to still exist that used to be manually del'd. The gc would now be able to reach the object and the del would raise an exception as the Structure object no longer exists.

tracemalloc setup

# trace.py

import os
import smbclient

test_file = os.path.join(
    os.path.abspath(os.path.dirname(__file__)), 'testingdata.bin'
)
dst = r'\\Server\Share\some\path'

if not os.path.isfile(test_file):
    # Create a 100 MB file for testing
    with open(test_file, 'wb') as f:
        _ = f.seek((100 * 1024 * 1024) - 1)
        _ = f.write(b'\0')

def run_io():
    with open(test_file, 'rb') as reader:
        with smbclient.open_file(dst, 'wb') as writer:
            while data := reader.read(4 * 1024 * 1024):
                writer.write(data)

# with_tracemalloc.py
import tracemalloc

import trace

tracemalloc.start(10)
time1 = tracemalloc.take_snapshot()
x = trace.run_io()
time2 = tracemalloc.take_snapshot()
stats = time2.compare_to(time1, 'lineno')
for i in stats[:10]:
    print(i)

tracemalloc output

Control (No modifications)

/smbprotocol/src/smbprotocol/structure.py:115: size=56.0 MiB (+56.0 MiB), count=18 (+18), average=3186 KiB
/smbprotocol/src/smbclient/_io.py:593: size=52.0 MiB (+52.0 MiB), count=13 (+13), average=4096 KiB
/smbprotocol/src/smbprotocol/connection.py:1280: size=48.0 MiB (+48.0 MiB), count=13 (+13), average=3781 KiB
/.local/lib/python3.14/site-packages/cryptography/hazmat/bindings/openssl/binding.py:51: size=145 KiB (+145 KiB), count=1182 (+1182), average=126 B
<frozen importlib._bootstrap_external>:511: size=123 KiB (+123 KiB), count=1692 (+1692), average=75 B
/.local/lib/python3.14/site-packages/cryptography/hazmat/bindings/openssl/binding.py:49: size=35.4 KiB (+35.4 KiB), count=585 (+585), average=62 B
/smbprotocol/src/smbprotocol/header.py:202: size=18.5 KiB (+18.5 KiB), count=270 (+270), average=70 B
/usr/local/lib/python3.14/stringprep.py:24: size=18.1 KiB (+18.1 KiB), count=2 (+2), average=9256 B
/smbprotocol/src/smbprotocol/header.py:258: size=18.0 KiB (+18.0 KiB), count=255 (+255), average=72 B
<frozen abc>:123: size=15.0 KiB (+15.0 KiB), count=161 (+161), average=95 B

Structure 62L was a weakref.proxy and thats it

/smbprotocol/src/smbclient/_io.py:593: size=100 MiB (+100 MiB), count=25 (+25), average=4096 KiB
/.local/lib/python3.14/site-packages/cryptography/hazmat/bindings/openssl/binding.py:51: size=145 KiB (+145 KiB), count=1182 (+1182), average=126 B
<frozen importlib._bootstrap_external>:511: size=123 KiB (+123 KiB), count=1699 (+1699), average=74 B
/.local/lib/python3.14/site-packages/cryptography/hazmat/bindings/openssl/binding.py:49: size=35.4 KiB (+35.4 KiB), count=585 (+585), average=62 B
/smbprotocol/src/smbprotocol/open.py:662: size=28.5 KiB (+28.5 KiB), count=375 (+375), average=78 B
/usr/local/lib/python3.14/stringprep.py:24: size=18.1 KiB (+18.1 KiB), count=2 (+2), average=9256 B
<frozen abc>:123: size=15.0 KiB (+15.0 KiB), count=162 (+162), average=95 B
/usr/local/lib/python3.14/stringprep.py:262: size=11.2 KiB (+11.2 KiB), count=98 (+98), average=117 B
/smbprotocol/src/smbprotocol/open.py:676: size=8800 B (+8800 B), count=75 (+75), average=117 B
/smbprotocol/src/smbprotocol/open.py:678: size=7400 B (+7400 B), count=50 (+50), average=148 B

Structure 62L as weakref.proxy and all lambdas converted to @staticmethods

/.local/lib/python3.14/site-packages/cryptography/hazmat/bindings/openssl/binding.py:51: size=144 KiB (+144 KiB), count=1166 (+1166), average=127 B
<frozen importlib._bootstrap_external>:511: size=124 KiB (+124 KiB), count=1715 (+1715), average=74 B
/.local/lib/python3.14/site-packages/cryptography/hazmat/bindings/openssl/binding.py:49: size=35.4 KiB (+35.4 KiB), count=585 (+585), average=62 B
/usr/local/lib/python3.14/stringprep.py:24: size=18.1 KiB (+18.1 KiB), count=2 (+2), average=9256 B
<frozen abc>:123: size=15.0 KiB (+15.0 KiB), count=162 (+162), average=95 B
/usr/local/lib/python3.14/stringprep.py:262: size=11.2 KiB (+11.2 KiB), count=98 (+98), average=117 B
/usr/local/lib/python3.14/enum.py:1507: size=6536 B (+6536 B), count=38 (+38), average=172 B
/.local/lib/python3.14/site-packages/cryptography/hazmat/backends/openssl/backend.py:31: size=3802 B (+3802 B), count=12 (+12), average=317 B
/usr/local/lib/python3.14/encodings/idna.py:370: size=3288 B (+3288 B), count=12 (+12), average=274 B
/usr/local/lib/python3.14/stringprep.py:220: size=2968 B (+2968 B), count=24 (+24), average=124 B

The control tracemalloc shows 156MB leaked memory for writing a 100MB file. Just by converting 62L Structure to a weakref.proxy, the memory leak drops down to 100MB for writing a 100MB file. By converting all of the self referential lambdas and 62L Structure to weakref.proxy, the leak is gone.

I also ran some tests against memray, which appear to corroborate with the changes.

Here is the memory graph output for the master branch

Compared to the modified version with @staticmethods and the weakref.proxy modification

[jborean93]

Thanks for the great analysis and potential fix. I would personally love to remove the whole structure setup I had going on here (I wrote it very early in my dev careet) but unfortunately it's part of the public API or at least people may be depending on it so it needs to stay for now. If your changes can guarantee that how you interact with the structure stays the same then I would love to see a PR to solve this problem and improve things!

[PyWoody]

The core use of Structure and Field and its subclasses shouldn't be changed by my modifications. All of the tests passed, too. Sorry, I should have mentioned that.

Do you know any edge cases or weird ways in which people may be (ab)using them that I should check?

The modifications shouldn't change any existing behavior but they do touch almost every file in smbprotocol, so I have no problem taking it slow.

If you think it's good-to-go, I'd be glad to make the PR and continue forward.

[jborean93]

Do you know any edge cases or weird ways in which people may be (ab)using them that I should check?

It's mostly that some attributes on the Connection/Session/TreeConnect/Open may store some of the structure values that aren't prefixed with _ so it'll be hard to tell what people are actually using it for. Ultimately the main things is that accessing the actual Python value the structure field represents stays the same and we should be all good.

Based on your description it sounds like it's more of an internal/implementation detail change to remove some extra object allocations which aren't needed at all in favour of some weak refs. If the tests pass without any major changes outside of testing the actual structure/fields tests then I think we'll be ok.

[PyWoody]

Yep. The test implementations and all references shouldn't be affected.

As an example, in SMB2CreateRequest, the "buffer_path" goes from

BytesField(size=lambda s: self._buffer_path_size(s))

to

BytesField(size=SMB2CreateRequest._buffer_path_size)

where _buffer_path_size is now a staticmethod. If an end user was using _buffer_path_size, they could still reference it via self like before.

Sorry for the delay. I had to get a test environment setup for the full lib::setup::smb_server test suite and it was just a calamity of issues (all on my end). I'll cleanup the code according to black and make the PR.

[jborean93]

No worries, take as much time as you need.