Closed
Bug 1257250
Opened 8 years ago
Closed 8 years ago
PR_GetNameForIdentity threadsafety and bound check fixes
Categories
(NSPR :: NSPR, defect)
NSPR
NSPR
Tracking
(Not tracked)
RESOLVED
FIXED
4.13
People
(Reporter: mcmanus, Assigned: mcmanus)
References
Details
Attachments
(1 file, 1 obsolete file)
1.44 KB,
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
PR_GetUniqueIdentity() takes care to be threadsafe in expanding the identity_cache table.. however that table is also accessed by PR_GetNameForIdentity() which does not do any locking. The latter could easily de-reference identity_cache.name[ident] after that memory had been freed by PR_GetUniqueIdentity() the "ident" argument is also a signed int and while it is bounds checked against the max size of the array it is not bounds checked for < 0.. and there are a number of 'special' layers that have negative identifies that could be easily be passed into this function and would result in a segv so its best to runtime bounds check it. Firefox doesn't currently use PR_GetUniqueIdentity() directly, but from bug reports at least one LSP does do so and this creates a crash risk.
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8731339 -
Flags: review?(ted)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → mcmanus
Status: NEW → ASSIGNED
Comment 2•8 years ago
|
||
Comment on attachment 8731339 [details] [diff] [review] pr_getnameforidentity bounds check and locking Review of attachment 8731339 [details] [diff] [review]: ----------------------------------------------------------------- ::: nsprpub/pr/src/io/prlayer.c @@ +692,4 @@ > > + if (ident < 0) { > + goto done; > + } Is there a reason you chose to use goto here instead of just wrapping these lines in an if block? Seems like this could easily be: if (PR_TOP_IO_LAYER != ident && ident >= 0) { ... }
Attachment #8731339 -
Flags: review?(ted)
Assignee | ||
Comment 3•8 years ago
|
||
Attachment #8733429 -
Flags: review?(ted)
Assignee | ||
Updated•8 years ago
|
Attachment #8731339 -
Attachment is obsolete: true
Updated•8 years ago
|
Attachment #8733429 -
Flags: review?(ted) → review+
Assignee | ||
Comment 4•8 years ago
|
||
thanks! how do I get this pushed to https://hg.mozilla.org/projects/nspr/ ?
Flags: needinfo?(ted)
Comment 5•8 years ago
|
||
I'll land it for you when I have a minute. Feel free to land this to inbound as well if you'd like.
Assignee | ||
Comment 6•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/56d3ab31480a27c4e6063f635f78625b8ce4d220 Bug 1257250 - pr_getnameforidentity bounds check and locking r=ted.mielczarek
Assignee | ||
Updated•8 years ago
|
Whiteboard: [leave open]
Assignee | ||
Comment 7•8 years ago
|
||
I've marked it leave open until ted gets a chance to check it into upstream. thanks!
Comment 8•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/56d3ab31480a
Comment 9•8 years ago
|
||
https://hg.mozilla.org/projects/nspr/rev/18fbd3af342b06747878452c4164b05428ba5a92 Bug 1257250 - pr_getnameforidentity bounds check and locking r=ted.mielczarek
Updated•8 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: needinfo?(ted)
Resolution: --- → FIXED
Assignee | ||
Updated•8 years ago
|
Whiteboard: [leave open]
Updated•8 years ago
|
Target Milestone: --- → 4.13
You need to log in
before you can comment on or make changes to this bug.
Description
•