Closed
Bug 175167
Opened 22 years ago
Closed 21 years ago
SEC_QuickDERDecodeItem should free memory upon failure
Categories
(NSS :: Libraries, defect, P2)
Tracking
(Not tracked)
RESOLVED
WONTFIX
3.7
People
(Reporter: julien.pierre, Assigned: wtc)
Details
Attachments
(1 file, 2 obsolete files)
1.74 KB,
patch
|
wtc
:
review+
rrelyea
:
superreview+
|
Details | Diff | Splinter Review |
When decoding fails, this function may have allocated memory in the arena. The memory is never leaked since eventually the arena will be freed. However, it is still allocated until that happens. I have noticed that we have a PORT_ArenaMark and PORT_ArenaRelease which can be used by this function to immediately release the memory that was allocated within it.
Reporter | ||
Comment 1•22 years ago
|
||
Assignee | ||
Comment 2•22 years ago
|
||
Comment on attachment 103280 [details] [diff] [review] use PORT_ArenaMark and PORT_ArenaRelease to free memory as needed >+ if (SECSuccess != rv) >+ { >+ PORT_ArenaRelease(arena, savpos); > } Just a nit: one reason for failure is that arena, templateEntry, or src is NULL. In that case, savpos is NULL (the initial value). Passing a NULL savpos to PORT_ArenaRelease causes it to malfunction. Passing a NULL arena to PORT_ArenaRelease causes it to dereference a NULL pointer. These (unlikely) problems can be avoided if you return from the function with SECFailure right away on bad input arguments. By the way, if src is NULL, you still dereference it in "newsrc = *src;".
Assignee | ||
Comment 3•22 years ago
|
||
Comment on attachment 103280 [details] [diff] [review] use PORT_ArenaMark and PORT_ArenaRelease to free memory as needed >+ if (SECSuccess != rv) >+ { >+ PORT_ArenaRelease(arena, savpos); > } Please ignore my previous comments on savpos being NULL because by the time we get here, the line savpos = PORT_ArenaMark(arena); has been executed. In any case, this patch (or rather the SEC_QuickDERDecodeItem function) needs work to be bullet-proof against NULL input arguments.
Attachment #103280 -
Flags: needs-work+
Reporter | ||
Comment 4•22 years ago
|
||
Assignee | ||
Comment 5•22 years ago
|
||
Comment on attachment 103369 [details] [diff] [review] patch to make SEC_QuickDERDecodeItem bullet-proof against NULL input arguments r=wtc. I would return with SECFailure right away on NULL input arguments to avoid the nested if statements. But I know this is the signature of your code :-)
Attachment #103369 -
Flags: review+
Reporter | ||
Comment 6•22 years ago
|
||
I checked in the fix to the NSS tip but forgot to mark this resolved.
Status: NEW → RESOLVED
Closed: 22 years ago
Priority: -- → P2
Resolution: --- → FIXED
Target Milestone: --- → 3.7
Reporter | ||
Comment 7•21 years ago
|
||
I'm reopening this bug. The approach chosen uses PORT_ArenaMark and PORT_ArenaRelease / PORT_ArenaUnmark . This method is is fact not thread-safe in many cases. It wil only work safely if the arena is known to only one thread. Therefore, we cannot use it in something as generic as the DER decoder. I'm going to attach a patch to undo the changes. Once reviewed, the bug should be either LATER or WONTFIX.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Reporter | ||
Comment 8•21 years ago
|
||
Attachment #103280 -
Attachment is obsolete: true
Attachment #103369 -
Attachment is obsolete: true
Reporter | ||
Updated•21 years ago
|
Attachment #116378 -
Flags: superreview?(relyea)
Attachment #116378 -
Flags: review?(wtc)
Assignee | ||
Comment 9•21 years ago
|
||
Comment on attachment 116378 [details] [diff] [review] Don't use mark & release in QuickDER r=wtc. The change to secdig.c is unrelated but is correct.
Attachment #116378 -
Flags: review?(wtc) → review+
Reporter | ||
Comment 10•21 years ago
|
||
The change to secdig made it inadvertedly because I did a cvs diff in the lib/util directory. I'll check it separately in its appropriate bug. Resolving this one as WONTFIX . Checking in quickder.c; /cvsroot/mozilla/security/nss/lib/util/quickder.c,v <-- quickder.c new revision: 1.18; previous revision: 1.17 done
Status: REOPENED → RESOLVED
Closed: 22 years ago → 21 years ago
Resolution: --- → WONTFIX
Updated•21 years ago
|
Attachment #116378 -
Flags: superreview?(rrelyea0264) → superreview+
You need to log in
before you can comment on or make changes to this bug.
Description
•