Closed Bug 403370 Opened 13 years ago Closed 13 years ago

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

Categories

(Core :: Networking, defect)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla1.9beta4

People

(Reporter: wtc, Assigned: wtc)

Details

(Keywords: memory-leak)

Attachments

(1 file)

Attached patch Proposed patchSplinter Review
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.
Attachment #288186 - Flags: superreview?(darin.moz)
Attachment #288186 - Flags: review?(cbiesinger)
Attachment #288186 - Flags: review?(cbiesinger) → review+
Darin isn't doing reviews anymore, no?
Comment on attachment 288186 [details] [diff] [review]
Proposed patch

OK, sr=me
Attachment #288186 - Flags: superreview?(darin.moz) → superreview+
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.
Attachment #288186 - Flags: approval1.9?
Status: NEW → ASSIGNED
Version: unspecified → Trunk
Comment on attachment 288186 [details] [diff] [review]
Proposed patch

a1.9+=damons
Attachment #288186 - Flags: approval1.9? → approval1.9+
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
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta4
You need to log in before you can comment on or make changes to this bug.