On Windows, freeaddrinfo(NULL) will result in a segv. In
get_recent_address(), there is the following block of code:
if(cache->aip) {
sockaddr_t *sa = (sockaddr_t *)cache->aip->ai_addr;
cache->aip = cache->aip->ai_next;
if(!cache->aip) {
freeaddrinfo(cache->aip);
cache->aip = NULL;
}
return sa;
}
where freeaddrinfo() is called when cache->aip is guaranteed to be
NULL. I get a bit confused with respect to cache->ai vs. cache->aip
but that part of the code looks suspicious.
At first I thought it was intended to free the old value of cache->aip
but since sa points to memory within that chunk, freeing it would
result in a use after free situation.
- todd
On Mon, Jan 22, 2018 at 09:51:33AM -0700, Todd C. Miller wrote:> On Windows, freeaddrinfo(NULL) will result in a segv. In > get_recent_address(), there is the following block of code: > > if(cache->aip) { > sockaddr_t *sa = (sockaddr_t *)cache->aip->ai_addr; > cache->aip = cache->aip->ai_next; > > if(!cache->aip) { > freeaddrinfo(cache->aip); > cache->aip = NULL; > } > > return sa; > } > > where freeaddrinfo() is called when cache->aip is guaranteed to be > NULL. I get a bit confused with respect to cache->ai vs. cache->aip > but that part of the code looks suspicious.Thanks for reporting this bug! Indeed, that code is wrong. It should free cache->ai instead of cache->aip. Even if it didn't crash on Linux, it was a memory leak. A fix is now in the git repository. -- Met vriendelijke groet / with kind regards, Guus Sliepen <guus at tinc-vpn.org> -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 833 bytes Desc: not available URL: <http://www.tinc-vpn.org/pipermail/tinc-devel/attachments/20180122/673ca4ba/attachment.sig>
On Mon, 22 Jan 2018 18:08:22 +0100, Guus Sliepen wrote:> Thanks for reporting this bug! Indeed, that code is wrong. It should > free cache->ai instead of cache->aip. Even if it didn't crash on Linux, > it was a memory leak. A fix is now in the git repository.Thanks for the quick fix. I noticed some other missing freeaddrinfo() calls. https://github.com/gsliepen/tinc/pull/171 - todd
On Mon, 22 Jan 2018 18:08:22 +0100, Guus Sliepen wrote:> Thanks for reporting this bug! Indeed, that code is wrong. It should > free cache->ai instead of cache->aip. Even if it didn't crash on Linux, > it was a memory leak. A fix is now in the git repository.The fix leads to a use after free bug which is visible on macOS. It doesn't seem to be much of a problem on most platforms since a copy of freed memory is made almost immediately but on macOS the memory appears to be cleared much of the time. This results in an attempt to connect to an AF_UNSPEC socket with no address, which of course fails. A potential fix, modeled on the "recently seen addresses not in our cache" part of get_recent_address() is: https://github.com/gsliepen/tinc/pull/177 Another approach would be to pass the sockaddr_t to get_recent_address() and fill it in there. I can write a diff for that if you'd like. I also noticed a problem with the recently seen but uncached code which was hidden by a cast as well as a buffer overflow (read, not write). The PR includes fixes for these two as well. - todd