search

droe / sslsplit

Transparent SSL/TLS interception
https://www.roe.ch/SSLsplit
BSD 2-Clause "Simplified" License
1.73k stars 327 forks source link

Fix some resource leaks #321

Closed disaykin closed 1 year ago

sonertari commented 1 year ago

I have cherry-picked https://github.com/droe/sslsplit/pull/321/commits/49b26345b578ed23039c543b8c88aceec60558ff and https://github.com/droe/sslsplit/pull/321/commits/8ec3becc3bad93f865cdd380240340909a8d30ba, but please review my changes to https://github.com/droe/sslsplit/pull/321/commits/ceedd458195f590ff10144ccd0e9bef2cedaee98, otherwise would lead to double free due to our (your) recent changes to sys_sockaddr_str().

If looking good to you too, you can close this pr, as I don't know how github closes prs if you just cherry-pick some of the commits.

sonertari commented 1 year ago

Wait, no, it wouldn't cause double free, I take that back. But what do you think about my changes?

And I guess I made a mistake by assuming the asprintf man page on Linux:

the contents of strp are undefined.

Because the OpenBSD asprintf(3) reads:

On OpenBSD, ret is set to the NULL pointer, but other implementations may leave ret unchanged.

disaykin commented 1 year ago

Your version looks good to me. Asprintf error handling is ugly in any case because the contents of strp are undefined in case of error. I think the code has another error: cbuf is reused in second asprintf, this can lead to memory leak.

sonertari commented 1 year ago

Yes, I was concerned about cbuf, fixed now, thanks.