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

tlstran_pipe_stop should free cparam #755

Merged
merged 2 commits into from Dec 8, 2023

Conversation

lkyjason
Copy link
Contributor

@lkyjason lkyjason commented Dec 7, 2023

fixes #1519 Several memleaks in TLS

NanoMQ issue: nanomq/nanomq#1519

p->tcp_cparam that is allocated in tlstran_pipe_nego_cb is not freed by tlstran_pipe_stop, instead the variable is directly set to NULL resulting in memory leak.

Signed-off-by: Jason Lee <lkyjason91@gmail.com>
@lkyjason lkyjason force-pushed the lkyjason/1519-conn-param-leak branch from 5da0982 to ee192e1 Compare December 7, 2023 02:14
@destroyer5656
Copy link

@OdyWayne @JaylinYu FYI for this issue

@OdyWayne
Copy link
Collaborator

OdyWayne commented Dec 7, 2023

Thank you for your contribution! But we usually do frees in pipe_fini not pipe_stop. We will appreciate it if you can move all these to tlstran_pipe_fini, just like what we do in tcptran_pipe_fini. Or should I do it for you?

Copy link
Collaborator

@OdyWayne OdyWayne left a comment

Choose a reason for hiding this comment

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

better do this in pipe_fini

Signed-off-by: Jason Lee <lkyjason91@gmail.com>
@lkyjason
Copy link
Contributor Author

lkyjason commented Dec 7, 2023

@OdyWayne moved to pipe_fini :)

@JaylinYu
Copy link
Member

JaylinYu commented Dec 8, 2023

Some modification we did on broker_tcp needs to be synced to broker_tls

@OdyWayne OdyWayne merged commit 559129e into nanomq:main Dec 8, 2023
7 checks passed
@OdyWayne
Copy link
Collaborator

OdyWayne commented Dec 8, 2023

well done

@lkyjason
Copy link
Contributor Author

lkyjason commented Dec 8, 2023

Thank you @OdyWayne

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.

None yet

4 participants