403370 - nsHostResolver::ResolveHost leaks a PRNetAddr whenever an IP address literal is parsed again

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Wan-Teh Chang]

Attachment: Proposed patch

This leak was introduced in the fix for bug 219376
(nsHostResolver.cpp, rev. 1.2).

In nsHostResolver::ResolveHost, we have:

            if (!he || !he->rec)
                rv = NS_ERROR_OUT_OF_MEMORY;
            // do we have a cached result that we can reuse?
            else if (!(flags & RES_BYPASS_CACHE) &&
                     he->rec->HasResult() &&
                     NowInMinutes() <= he->rec->expiration) {
                LOG(("using cached record\n"));
                // put reference to host record on stack...
                result = he->rec;
            }
            // try parsing the host name as an IP address literal to short
            // circuit full host resolution.  (this is necessary on some
            // platforms like Win9x.  see bug 219376 for more details.)
            else if (PR_StringToNetAddr(host, &tempAddr) == PR_SUCCESS) {
                // ok, just copy the result into the host record, and be done
                // with it! ;-)
                he->rec->addr = (PRNetAddr *) malloc(sizeof(PRNetAddr));
                if (!he->rec->addr)
                    status = NS_ERROR_OUT_OF_MEMORY;
                else
                    memcpy(he->rec->addr, &tempAddr, sizeof(PRNetAddr));
                // put reference to host record on stack...
                result = he->rec;
            }

Since we do not set |expiration| of the record for an IP address
literal, the record is always considered expired (|expiration| is
still the initial value, the creation time of the record), so we
always hit the PR_StringToNetAddr case.

Note that we do not free the old value of he->rec->addr before
assigning the new value to it.  So the old PRNetAddr is leaked.

To reproduce the leak, visit any website using its IP address and
click on a few links within the website.

The proposed patch fixes the leak.  It also removes a strange
comparison with nsnull in nsHostRecord::HasResult().

I considered setting he->rec->expiration to PR_UINT32_MAX so that
the record never expires, but it would only work when
!(flags & RES_BYPASS_CACHE) is true because the expiration check
is only performed when that condition is true.

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Darin isn't doing reviews anymore, no?

Comment 2

[Darin Fisher]

Comment on attachment 288186 [details] [diff] [review]
Proposed patch

OK, sr=me

Comment 3

[Wan-Teh Chang]

Comment on attachment 288186 [details] [diff] [review]
Proposed patch

Requesting approval1.9 for FF 3 Beta 4.  This patch fixes
a memory leak when we visit a URL with the host specified
with an IP address rather than a DNS name.

Comment 4

[Damon Sicore (:damons)]

Comment on attachment 288186 [details] [diff] [review]
Proposed patch

a1.9+=damons

Comment 5

[Wan-Teh Chang]

I checked in the patch on the trunk for FF 3 Beta 4.

Checking in nsHostResolver.cpp;
/cvsroot/mozilla/netwerk/dns/src/nsHostResolver.cpp,v  <--  nsHostResolver.cpp
new revision: 1.22; previous revision: 1.21
done
Checking in nsHostResolver.h;
/cvsroot/mozilla/netwerk/dns/src/nsHostResolver.h,v  <--  nsHostResolver.h
new revision: 1.10; previous revision: 1.9
done