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

Setting TLS config option via nng_socket_set_ptr causes access violation if you free config #1835

Open
DoumanAsh opened this issue May 29, 2024 · 3 comments

Comments

@DoumanAsh
Copy link

DoumanAsh commented May 29, 2024

Describe the bug
Subj

Expected behavior
Perform increment of refcount within setter function?

Actual Behavior
User is required to make shallow copy of tls config

To Reproduce
If possible include actual reproduction test code here.
Minimal C test cases are perferred.

Environment Details

  • NNG version - 1.8.0
  • Operating system and version - Windows 10
  • Compiler and language used - C (to compile nng)/Rust(bindings)
  • Shared or static library - static

Additional context

Dialer/Listener setters would increment refcount, but it seems that's not the case when using socket option as it causes use after free according to Dr.Memory
But I suspect TLS is not properly supported to be set via nng_socket_set_ptr anyway.
Still I thought to report it as this behavior is not very well documented

@DoumanAsh DoumanAsh changed the title Setting TLS option via nng_socket_set_ptr causes access violation if you free config Setting TLS config option via nng_socket_set_ptr causes access violation if you free config May 29, 2024
@gdamore
Copy link
Contributor

gdamore commented Jun 1, 2024

Thank you for the bug report. I will investigate... there is going to be more work in this general area anyway -- but definitely its the case that the TLS configuration struct does not "borrow" a reference counter.

@DoumanAsh
Copy link
Author

Oh I see, so setting TLS config on socket itself is supported?
Because I remember seeing no special handling for it in setter https://github.com/nanomsg/nng/blob/master/src/core/socket.c#L1048

If it is supported that is definitely needs to be fixed to increment refcount 😄

@gdamore
Copy link
Contributor

gdamore commented Jun 8, 2024

The caller has to keep the configuration around for now. Which is unfortunate. The problem is that with some of these configuration objects we don't have a good way to clone them, because they are linked to the configuration of the underlying TLS library which might have e.g. linked lists, etc.

I'm going to refactor this to have "native" NNG configuration data instead I think.

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

No branches or pull requests

2 participants