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

Do not segfault when closing last connection #83

Merged
merged 1 commit into from Apr 6, 2022
Merged

Do not segfault when closing last connection #83

merged 1 commit into from Apr 6, 2022

Conversation

vicencb
Copy link
Contributor

@vicencb vicencb commented Feb 4, 2022

When there is a single item in the queue, queue_next_elem returns that same and only item, so, in the function conn_close, nextconn is conn.
Afterwards, conn is deleted, which is also nextconn, so, conn_close returns a pointer to a freed connection.
This patch sets the return value of conn_close to NULL when there are no more connections.

@3cky
Copy link
Owner

3cky commented Feb 17, 2022

Hello @vicencb!

I see your point and did some mbusd stress testing with concurrent client connections opening/closing, but still can't get a segfault.

Could you provide some steps to reproduce the problem in practice?

@vicencb
Copy link
Contributor Author

vicencb commented Feb 19, 2022

Hello @3cky,
dereferencing a pointer to freed memory is undefined behavior. In my case this behavior showed up as a segmentation fault, your case is different probably because of different compiler or other involved parts.

You can still see the issue by compiling the program with -fsanitize=address which is supported by both gcc and clang or else you can run mbusd with valgrind.

In order to test it, there is no need to use concurrent connections. Just do one TCP connection and then disconnect it.

@dgoo2308
Copy link
Contributor

dgoo2308 commented Apr 6, 2022

@3cky the logic is sound on this request!

@3cky
Copy link
Owner

3cky commented Apr 6, 2022

Valgrind did the trick. I was able to reproduce the use-after-free behavior and I confirm that this PR fixes it.

LGTM, will merge. Thanks!

@3cky 3cky merged commit 874ce63 into 3cky:master Apr 6, 2022
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

3 participants