351580 - Possible null pointer dereferences in |nsCrypto::GenerateCRMFRequest|

Status: RESOLVED FIXED

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

Description

[Kenneth Herron]

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.

Comment 1

[timeless]

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.

Comment 2

[Mike Schroepfer]

+ing - comment 1 sounds a little scary.  Kai you have cycles for this?

Comment 3

[Kai Engert (:KaiE:)]

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?

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 5

[Honza Bambas (:mayhemer)]

Attachment: Fixed !NULL checks + GC rooting, v1

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.

Comment 6

[Reed Loden [:reed]]

Comment on attachment 306343 [details] [diff] [review]
Fixed !NULL checks + GC rooting, v1

This is a blocker. No need for approval.

Comment 7

[Reed Loden [:reed]]

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