Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix memory leak (free the descriptor) #3487

Merged
merged 1 commit into from
Dec 10, 2024

Conversation

m08pvv
Copy link
Contributor

@m08pvv m08pvv commented Dec 9, 2024

Noticed a memory leak in record.c, here is a small patch.
If someone needs libasan trace log with specific lines that pointed to this descriptor, I can post it here or paste it somewhere (it's a nice 8 lines stacktrace from which you only need 3.5)

@januscla
Copy link

januscla commented Dec 9, 2024

Thanks for your contribution, @m08pvv! Please make sure you sign our CLA, as it's a required step before we can merge this.

@m08pvv m08pvv changed the title free the descriptor Fix memory leak (free the descriptor) Dec 9, 2024
@ugenef
Copy link

ugenef commented Dec 10, 2024

@lminiero
Hey,
we're facing the same issue. Looks like the PR is going to fix that.

@lminiero
Copy link
Member

If someone needs libasan trace log with specific lines that pointed to this descriptor, I can post it here or paste it somewhere

No need, it's a pretty obvious leak, thanks for spotting it! I think it went unnoticed so far because it's basically never used: only the VideoRoom adds a description, but only if a publisher stream has a description, which I think our stock demos never add, and probably not many people using the plugin do either. I'll merge and backport to 0.x.

@lminiero lminiero merged commit 4419baf into meetecho:master Dec 10, 2024
8 checks passed
lminiero added a commit that referenced this pull request Dec 10, 2024
This reverts commit 932b696, since we
actually don't support the description flag in the 0.x recorder.
@lminiero
Copy link
Member

Actually, no change needed in 0.x: there's no description in the recorder there.

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