droe / sslsplit

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

log.c: fix double free #319

Closed disaykin closed 1 year ago

disaykin commented 1 year ago

Bug found by Svace static analyzer:

Pointer 'pem' is passed to a function free at log.c:1468 by calling function 'free' after the referenced memory was deallocated at logbuf.c:54 by passing as 1st parameter to function 'logbuf_new' at log.c:1464

sonertari commented 1 year ago

Thanks again. Svace seems like a useful tool, but it apparently misses a few similar double-free bugs in our code.

logbuf_new() is called from two other functions:

If you search for the places where logger_print_freebuf() is used, you'll find the log_connect_print_free() macro. And then if you search for the places where log_connect_print_free() is called, you'll find two places in pxyconn.c:

And in both cases msg buf is freed upon error, which is the same bug as svace has found.

It seems like svace cannot handle macros well. What about function pointers (we use function pointers a lot in certain parts of the code)? Does svace handle them well? Or is it a config option?

I was able to manually find only those additional cases yet, I'll keep searching. And I think those are some of the most difficult bugs to catch. Do you think you can use svace to search for all such cases?

Svace does not seem FOSS, or is it? Where do you download it, if possible at all?

disaykin commented 1 year ago

I plan to fix all the errors found using the Svace utility. As far as I know, Svace is not FOSS yet.

disaykin commented 1 year ago

@sonertari I couldn't find more than you did.

sonertari commented 1 year ago

Looking good to me, including your refactoring in logger.c. Thanks.