Conversation
Free the url field when resetting the stream to avoid leaking it.
Don't just free the push status structure, actually free the strings that were strdup'd into the struct as well.
Don't just free the spec vector, also free the specs themselves.
Instead of allocating the ciphers_list, make it a static array. This prevents us from leaking it or having to manage its memory.
|
Additionally, when running valgrind on a build with mbedTLS, I saw some additional leaks. |
tiennou
left a comment
There was a problem hiding this comment.
Looks good to me, thanks !
I'm slightly ashamed of being responsible for leaks. I forgot that the push suite is disabled on default runs…
No problem - I'm proposing that we enable leak detection on all test runs on all builds, which is where I found it. |
|
This has already been backported partially, excluding the mbedtls changes as this backend doesn't yet exist in v0.27. Removing the backport label |
|
I'm not sure this should have been backported, it seems this has caused some API behavior breakage. In particular, this breaks Cargo's ability to fetch repositories, specifically https://github.com/alexcrichton/git2-rs/blob/master/src/transport.rs#L223 which is fixed after reverting 49ee0ae. See also rust-lang/cargo#6365 |
|
The built-in transports store a partsed version of the url themselves during initialisation and ignore the URL passed in in This does look like a bug in the aforementioned commit. We run |
When running valgrind on the
online::pushtests, some leaks became apparent. Two in the tests themselves, one in the product code that occurs when restarting a push on an existing remote.