Closed
Bug 403370
Opened 16 years ago
Closed 15 years ago
nsHostResolver::ResolveHost leaks a PRNetAddr whenever an IP address literal is parsed again
Categories
(Core :: Networking, defect)
Core
Networking
Tracking
()
RESOLVED
FIXED
mozilla1.9beta4
People
(Reporter: wtc, Assigned: wtc)
Details
(Keywords: memory-leak)
Attachments
(1 file)
2.55 KB,
patch
|
Biesinger
:
review+
darin.moz
:
superreview+
damons
:
approval1.9+
|
Details | Diff | Splinter 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)
Updated•15 years ago
|
Attachment #288186 -
Flags: review?(cbiesinger) → review+
Comment 1•15 years ago
|
||
Darin isn't doing reviews anymore, no?
Comment 2•15 years ago
|
||
Comment on attachment 288186 [details] [diff] [review] Proposed patch OK, sr=me
Attachment #288186 -
Flags: superreview?(darin.moz) → superreview+
Assignee | ||
Comment 3•15 years ago
|
||
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?
Updated•15 years ago
|
Status: NEW → ASSIGNED
Version: unspecified → Trunk
Comment 4•15 years ago
|
||
Comment on attachment 288186 [details] [diff] [review] Proposed patch a1.9+=damons
Attachment #288186 -
Flags: approval1.9? → approval1.9+
Assignee | ||
Comment 5•15 years ago
|
||
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: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta4
You need to log in
before you can comment on or make changes to this bug.
Description
•