search

nanomsg / nng

nanomsg-next-generation -- light-weight brokerless messaging
https://nng.nanomsg.org
MIT License
3.78k stars 485 forks source link

setting NNG_OPT_TLS_CONFIG on socket should hold reference #1835

Closed DoumanAsh closed 2 months ago

DoumanAsh commented 4 months ago

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

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

gdamore commented 4 months ago

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 commented 4 months ago

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 commented 3 months ago

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.

gdamore commented 2 months ago

Going back at this... the TLS configuration should have had a borrow / reference count. If you set this on an endpoint rather than a socket, it does in fact get bumped. I'll check into this.

gdamore commented 2 months ago

One other thing:

Setting TLS options (and indeed transport options) on sockets is not the recommended approach. For example, you cannot have both a dialer and a listener using TLS if you set the socket option for it, because the options would be incompatible.

It is planned to eliminate transport configuration at the socket level entirely -- you really should be creating dialers or listeners, and configuring those, if you need this level of configuration.

DoumanAsh commented 2 months ago

Yes, I refactored my wrapper to use dialer/listeners in the end! Nevertheless it is good that it is fixed and no longer crashes