search

uNetworking / uSockets

Miniscule cross-platform eventing, networking & crypto for async applications
Apache License 2.0
1.29k stars 267 forks source link

Fixes SSL-related memory leaks #105

Closed d34d10cc closed 4 years ago

d34d10cc commented 4 years ago

SSL options copied with deep_str_copy in us_create_socket_context were not being properly freed (detected with Valgrind), because they were not being properly copied over to the context when SSL was enabled.

Relatively minor leaks; nevertheless, good to patch them up.

ghost commented 4 years ago

Can you explain more? I don't see how this fixes anything?

d34d10cc commented 4 years ago

Sure. At this line, the code path goes into the OpenSSL/WolfSSL implementations where on this line an internal context is created with an empty options struct. The function goes on to properly use the original options passed in from us_create_socket_context, but never copies them into the context created with the empty options struct, which it returns to us_create_socket_context, which in turn returns it immediately, without going on to the next block where the options are properly copied over into the created context in the non-SSL case. This leaves the options alloc'd in deep_str_copy at the beginning of the function without references, so they don't get cleaned up later in us_socket_context_free. Everything works properly, but the initial options used to set up the context are leaked.

Multiple ways to fix this, I just chose the one with the least code changes and also to avoid setting e.g. these options two times in the SSL case. The change in us_socket_context_free is to avoid a double free in the SSL case. (A little bit of a verbose explanation, just following my train of thought :)

ghost commented 4 years ago

Alright I have made some bigger changes that covers this also. It doesn't seem like a memory leak, rather a recursion bug which probably added the context two times in the loop's list. This is fixed now, thanks for reporting & proposing a fix!