On Fri, Jun 15 2018, Thomas Barabosch <[email protected]> wrote: > Hi there, > > yesterday I stumbled upon a memory leak in route6d, which got fixed. I > was curious if I could find similar leaks. Please correct me if I am > wrong but I suppose static analysis tools like coverity or clang > analyzer do not detect this kind of bug since they do not know anything > about the semantics of getaddrinfo/freeaddrinfo. > > So I wrote a small script to check all .c-files that call getaddrinfo > but do not call freeaddrinfo. Note that some of them may be false > positives, i.e. the addrinfo struct is actually free'd in another source > file. I had a look at some of the cases (but not all!). I've appended > the output to the end of this email. > > usr.bin/ssh/servconf.c > > I think that a leak in usr.bin/ssh/servconf.c should be plugged. In > function add_one_listen_addr, there is a call to getaddrinfo. This > function is called in a loop in function add_listen_addr. So in this > case that should leak memory depending on the number of loop iterations, > right?
Doesn't look like a leak, the resulting list is pushed on top of the previously stored ones. > -------------------------------------------------------------------------------- > > libexec/mail.local/mail.local.c > > In the case of mail.local, you may argument that this program terminates > rather quickly and the memory is free'd by the kernel anyways. But it > would be easy to fix. Variable "res0" is static, the code wants to cache the getaddrinfo result. I see no reason to add complexity to free this result at exit time. > -------------------------------------------------------------------------------- > > openbsd/usr.sbin/ndp/ndp.c > > Same as mail.local, easy to fix. Already fixed by your diff. > -------------------------------------------------------------------------------- > > usr.sbin/npppd/common/net_utils.c > > Seems to be a false positive. and the function looks unused. > -------------------------------------------------------------------------------- > > usr.sbin/smtpd/smtpc.c > > Static variable, should be free'd once the program terminates. Yep. > -------------------------------------------------------------------------------- > > usr.sbin/ikectl/parser.c > > getaddrinfo is called in function parse_addr (line 267). This function > is called by function match_token (line 284), which happens in a > for-loop. In this for loop, there is a case-matching. If the token > matches ADDRESS or FQDN, parse_addr is called twice each time. So it > leaks depending on the number of ADDRESS + FQDN tokens multiplied by two > times. Fixed proposed by benno@ > -------------------------------------------------------------------------------- > > usr.sbin/radiusd/util.c > > False positive. getaddrinfo is called in util function addrport_parse > and the addrinfo struct is returned by this function. However, it seems > to be properly free'd in radiusd_radius.c. Yep. > -------------------------------------------------------------------------------- > > Could you please have a look at these cases? May be I am missing > something here! Please note that the above method is very raw. It just > greps for strings, there is no (sophisticated) static analysis involved > similar to modules for malloc/free issue detection. Nevertheless, I > thought that this may be of interest to you. raw but it's already a good start. :) -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
