Skip to content

Fix memory leak in Raw.copy(). - #709

Merged
jcrist merged 2 commits into
msgspec:mainfrom
tjgq:leak
Oct 13, 2024
Merged

Fix memory leak in Raw.copy().#709
jcrist merged 2 commits into
msgspec:mainfrom
tjgq:leak

Conversation

@tjgq

@tjgq tjgq commented Jul 12, 2024

Copy link
Copy Markdown
Contributor

Raw_New borrows a reference to its PyBytesObject argument, so Raw_Copy must still dispose of it.

The following is a minimal repro for this issue:

import msgspec

DECODER = msgspec.msgpack.Decoder(type=msgspec.Raw)
PAYLOAD = b"\xda\xff\xff" + (65535 * b"\0") # array of 65535 zeros

for _ in range(1000000):
  raw = DECODER.decode(PAYLOAD)
  raw.copy()

@tjgq
tjgq marked this pull request as ready for review July 12, 2024 19:15
@tjgq

tjgq commented Jul 15, 2024

Copy link
Copy Markdown
Contributor Author

@jcrist could you please take a look?

tjgq and others added 2 commits October 13, 2024 12:41
Raw_New borrows a reference to its PyBytesObject argument, so Raw_Copy must
still dispose of it.

The following is a minimal repro for this issue:

import msgspec

DECODER = msgspec.msgpack.Decoder(type=msgspec.Raw)
PAYLOAD = b"\xda\xff\xff" + (65535 * b"\0") # array of 65535 zeros

for _ in range(1000000):
  raw = DECODER.decode(PAYLOAD)
  raw.copy()
@jcrist
jcrist merged commit 51a261c into msgspec:main Oct 13, 2024
@jcrist

jcrist commented Oct 13, 2024

Copy link
Copy Markdown
Member

Thanks!

pull Bot pushed a commit to Future-Outlier/msgspec that referenced this pull request Jul 2, 2026
Fixes msgspec#1108.

`Ext_New` doesn't steal the reference to `data` (it does its own
`Py_INCREF`, matching the borrow contract expected by the `Ext`
constructor). Both decode branches in `mpack_decode_ext` (typed `Ext`
decode, and `Any` decode without an `ext_hook`) passed a fresh reference
from `PyBytes_FromStringAndSize` and never released it, leaking one
bytes object (the ext payload) per decoded Ext. The `datetime` (code -1)
and `ext_hook` branches were not affected.

The fix releases the caller's reference after constructing the `Ext`,
same pattern as the `Raw.copy()` leak fix (msgspec#709). Verified empirically:
RSS delta over 200k decodes of a 1 KiB payload went from ~205 MiB to 0
on both affected branches; the new test fails before the fix
(`sys.getrefcount` 3 vs 2) and passes after.

Thanks @K-ANOY for the precise report, the ownership analysis there was
spot on.

Co-authored-by: Siyet <Siyet@users.noreply.github.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.

2 participants