Closed Bug 351580 Opened 14 years ago Closed 12 years ago

Possible null pointer dereferences in |nsCrypto::GenerateCRMFRequest|

Categories

(Core :: Security: PSM, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9beta5

People

(Reporter: kherron+mozilla, Assigned: mayhemer)

References

(Blocks 1 open bug, )

Details

(Keywords: coverity)

Attachments

(1 file)

This is coverity ID 1016. Please see the sample URL. |nsCrypto::GenerateCRMFRequest| makes several calls to |JS_ValueToString| without checking the return value for NULL. |JS_ValueToString| calls |js_ValueToString| which may return NULL in some circumstances.

Coverity flagged five different call sites between lines 1475-1505.
afaik this code also runs entirely afoul of js gc rooting, each string appears to be temporarily requested w/o being rooted and then the only root reference to it is forgotten and somehow this code expects the char* pointers from it to survive long enough while it proceeds to repeat this pattern and risk gc's numerous times.
QA Contact: psm
Flags: blocking1.9?
+ing - comment 1 sounds a little scary.  Kai you have cycles for this?
Flags: blocking1.9? → blocking1.9+
Priority: -- → P3
I could come up with a patch for comment 0.

Regarding comment 1, I don't understand the term "js gc rooting", so I don't know what to do to fix it. Help, pointers, or a more detailed explanation definitively welcome.

Timeless, could you elaborate, what are the variable names you are referring to? Which variable is the root reference that is being forgotton?
timeless is referring to the fact that in an exact GC model, you must protect all JS values you use in C against GC explicitly because the JS collector doesn't know about them. See http://developer.mozilla.org/en/docs/SpiderMonkey_Garbage_Collection_Tips as a reference.
This is fixed according to G_C_Types documentation. I also added requests around callback evaluation and root removal in dtor. CHECK_REQUEST assertion was failing.
Attachment #306343 - Flags: review?(mrbkap)
Assignee: kengert → honzab
Status: NEW → ASSIGNED
Attachment #306343 - Flags: review?(mrbkap) → review+
Flags: tracking1.9+ → blocking1.9+
Priority: P3 → P2
Comment on attachment 306343 [details] [diff] [review]
Fixed !NULL checks + GC rooting, v1

This is a blocker. No need for approval.
Attachment #306343 - Flags: approval1.9?
Checking in security/manager/ssl/src/nsCrypto.cpp;
/cvsroot/mozilla/security/manager/ssl/src/nsCrypto.cpp,v  <--  nsCrypto.cpp
new revision: 1.78; previous revision: 1.77
done
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
OS: Windows XP → All
Hardware: PC → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9
Target Milestone: mozilla1.9 → mozilla1.9beta5
You need to log in before you can comment on or make changes to this bug.