droe / sslsplit

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

A memory leak caused by function logbuf_new_printf() #276

Closed lc3412 closed 3 years ago

lc3412 commented 3 years ago

Hi,

There is a memory leak caused by function logbuf_new_printf(). See the following code, at line 1111/1113, function logbuf_new_printf() allocates the resource.

Consider this execuation trace: line 1111/1113 -> line 1120 -> line 1123 -> line 1129 -> line 1132 -> line 1133 -> ... -> line 1137, which means

  1. function logbuf_new_printf() successfully allocates the resource (line 1111/1113);

  2. then the return value of logbuf_new_printf() [pointer head] is assigned to the pointer lb (line 1120), that means pointer head and lb both point to the same resource allocated by logbuf_new_printf();

  3. function logbuf_new_copy() successfully allocates the resource , and the corresponding return value is assigned to the pointer head (line 1123);

  4. then the pointer head is still assigned to the pointer lb (line 1129), that means pointer head and lb both point to the same resource allocated by logbuf_new_copy(). Therefore, the resource that allocated by function logbuf_new_printf() is not pointed by any pointer.

  5. function logbuf_new_alloc() allocates the resource (line 1132), however, the allocating process is not successful, then the corresponding Error Handling actions are done (line 1134 - line 1136). As you can see, the action logbuf_free(lb) (line 1135) only free the resource allocated by function logbuf_new_copy(). However, the resource that allocated by function logbuf_new_printf() is overlooked.

https://github.com/droe/sslsplit/blob/9bac829b7f62252a8958b60c96f8f4b11125fcc9/log.c#L1110-L1137

Thanks so much for your patience.

Sincerely, Chi Li

sonertari commented 3 years ago

Hi, actually no, I don't see a memory leak there as you describe it, because the lb var is added to the linked list in logbuf_new_copy(). See what that function is doing, and note that lb is passed to it and used as a local var called next. The same is true for the function logbuf_new_alloc() too. So, whatever logbuf_new_printf() allocates will be freed by the helper functions for the linked list, after we are done with that list element of course.

But thanks for the inspection, and no problem, you are welcome to send your inspection results (correct or not). Code inspections as you are doing are valuable, but you might want to consider using tools like valgrind too, as they are good at catching such possible memory leaks.

lc3412 commented 3 years ago

Thanks for your explanation and suggestion!