uvw::TCPHandle::clear will also clear internal callbacks

[blattersturm]

In our application, we used uvw::TCPHandle::clear to clear all handlers prior to calling uvw::TCPHandle::close, so that closing would not invoke any of our application callbacks anymore.

However, if any uvw::details::WriteReq was still allocated (i.e. a uv_write was still pending, for instance with a malicious attacker application that will write but never read), the handler for this event will also be removed, and the subsequent behavior from uv_close will not invoke the internal WriteReq callback, and therefore will leak both the uv_write_t, any related referenced objects, as well as the std::unique_ptr<char[]>.

As an application author, we expect calling clear to not remove any handlers used internally by the library for memory management - maybe adding a flag to such internally-placed handlers would be helpful?

If it helps, this was tested on Windows, and our application uses the pipe-sharing method for forwarding TCP handles across threads so triggers the special-cased behavior in uv_close.

[skypjack]

It's enough to remove listeners on errors, then invoke close. Afaik the underlying library blocks all requests and handles and the pending requests return errors because of this. If you ignore them as it ought to be, everything works as expected in your case.

[blattersturm]

We are actually expecting pending requests to return errors, but clear() will in fact remove the internal handlers also, and that seems a bit counterintuitive. :/

Sent from ProtonMail Mobile

On Sun, Aug 9, 2020 at 19:48, Michele Caini notifications@github.com wrote:

It's enough to remove listeners on errors, then invoke close. Afaik the underlying library blocks all requests and handles and the pending requests return errors because of this. If you ignore them as it ought to be, everything works as expected in your case.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[skypjack]

Oh, well, in this case you don't have to clear handles. This boils down to how libuv works though. You can check the documentation for uv_close to know what happens in this case. It's easy to figure out how it maps to uvw then. If receiving errors is what you want, you don't have to disconnect the other listeners because they won't be invoked anyway. Dunno if it's counterintuitive, I think this is highly subjective. From my point of view, if I closed a handle I'd expect all pending requests to fail probably but I think I'm a little biased, so not a good feedback.

[blattersturm]

I'm sorry if I wasn't clear - uvw sometimes registers handlers internally such as for freeing a uvw::detail::WriteReq.

If I call clear() - such as for easier bookkeeping in our code - while such a request is pending, even if we don't register a handler for the Write or Error events ourselves, the library will leak memory.

Sent from ProtonMail Mobile

On Sun, Aug 9, 2020 at 20:10, Michele Caini notifications@github.com wrote:

Oh, well, in this case you don't have to clear handles. This boils down to how libuv works though. You can check the documentation for uv_close to know what happens in this case. It's easy to figure out how it maps to uvw then. If receiving errors is what you want, you don't have to disconnect the other listeners because they won't be invoked anyway. Dunno if it's counterintuitive, I think this is highly subjective. From my point of view, if I closed a handle I'd expect all pending requests to fail probably but I think I'm a little biased, so not a good feedback.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[skypjack]

Yeah, I see. clear is meant to discard all listeners of a handle that is about to be recycled and is not active. If you use it on an active handle, something bad will happen. That's why on and once return connection objects to use to disconnect listeners selectively. You can use them on active handles. To do what you suggest, that is, to differentiate the internal handles from the ones provided by users, we should either duplicate the emitter or use an extra parameter. This means more memory usage in all cases.

[blattersturm]

Right. The suggested solution should work fine, perhaps this gotcha should be mentioned somewhere though.. wasn't fun to debug the associated lifetime with some of these. :)

Sent from ProtonMail Mobile

On Sun, Aug 9, 2020 at 20:24, Michele Caini notifications@github.com wrote:

Yeah, I see. clear is meant to discard all listeners of a handle that is about to be recycled and is not active. If you use it on an active handle, something bad will happen. That's why on and once return connection objects to use to disconnect listeners selectively. You can use them on active handles. To do what you suggest, that is, to differentiate the internal handles from the ones provided by users, we should either duplicate the emitter or use an extra parameter. This means more memory usage in all cases.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[skypjack]

We could also intercept clear and assert in debug mode if the handle is active. At least this should make the bell ring for the developer.

I'm sorry if you had problems to track this down though.