Skip to content

fix malloc/mem.xfree inconsistency in isSameFuncLiteral - #23558

Merged
thewilsonator merged 1 commit into
dlang:masterfrom
rainers:isSameFuncLit
Aug 10, 2026
Merged

fix malloc/mem.xfree inconsistency in isSameFuncLiteral#23558
thewilsonator merged 1 commit into
dlang:masterfrom
rainers:isSameFuncLit

Conversation

@rainers

@rainers rainers commented Aug 8, 2026

Copy link
Copy Markdown
Member

OutBuffer uses C malloc, but memory is released with mem.xfree

This can cause memory leaks when mem.xfree is using the GC.

Moving the OutBuffer up the call tree keeps memory management encapsulated.

* The serialization of `fld` allocated with mem.
*/
private string getSerialization(FuncLiteralDeclaration fld, Scope* sc)
private bool getSerialization(FuncLiteralDeclaration fld, Scope* sc, ref OutBuffer buf)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. I suggest changing buf from ref to out, so that the return value is well-defined - it could wrongly return true if the buffer wasn't initially empty, or clear out any existing content in that buffer if the serialization fails. And ideally the Returns: docs would be updated too.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm not a fan of out parameters because they overwrite struct instances without checking whether they contain anything to cleanup, e.g. passing an OutBuffer with allocated data would cause a leak.

So I kept the ref but only return true if anything was appended. Updated the documentation, too.

@kinke kinke Aug 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I see a bunch of buf.length == 0 and buf.setsize(0) in the visitor, so this is clearly not suited for a non-empty buffer. Edit: So an assertion is IMO enough if you wanna keep the ref.

@rainers rainers Aug 10, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Indeed, I was probably assuming "serialization" was just appending. All content is cleared on error AFAICT, though that seems rather brittle (some writes are unconditonal, e.g. the closing ')' of a call).
I changed it to reset the buffer initially.

OutBuffer uses C malloc, but memory is released with mem.xfree
@thewilsonator
thewilsonator merged commit b7a5f0c into dlang:master Aug 10, 2026
42 checks passed
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.

3 participants