336995 - Coverity crash [@ header_length - der_encode - DER_encode] "dtemplate->sub" Pointer dereferenced before NULL check

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[timeless]

The DER_POINTER case null checks, but the DER_INLINE case does not.

Comment 1

[timeless]

Attachment: look before leaping like the other children

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 221198 [details] [diff] [review]
look before leaping like the other children

r=nelson

Comment 3

[timeless]

Unfortunately there are other instances in this file (I figure I'll try to limit the number of bugs we use to patch similar problems):

CID 496
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/util/derenc.c&rev=1.2&mark=346,353,355-356,371-375,378-379,383,386-387,390,393-394,397,398-399#382
Crash [@ der_encode - DER_encode] Variable "tmpt" tracked as NULL was dereferenced.
CID 495
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/util/derenc.c&rev=1.2&mark=202,206-207,223,227,230-231,235,241-242,245,247,248#200
Crash [@ contents_length] Variable "tmpt" tracked as NULL was dereferenced.

If you'd rather new bugs for these, feel free to indicate before the weekend (most likely my patch sprees will be on sundays).

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Timeless,  You know what PORT_Assert does (right?)
Will PORT_Assert suffice to silence coverity WRT the issued in comment 3
(e.g. PORT_Assert(tmpt != NULL); ) ?

Comment 5

[timeless]

as long as coverity continues to build debug, it should.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

taking.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Add 3 assertions, whose purpose is to silence covreity

I am extremely reluctant to make any last-minute real changes to the DER 
encoder logic, even (especially!) to silently avoid NULL ptr dereferences.  

So, I have added 3 PORT_Assert calls here.  
According to Timeless, these should silence coverity.
That is really their only present purpose. 

Alexei + Wan-Teh, please review.

Comment 8

[Wan-Teh Chang]

Comment on attachment 222451 [details] [diff] [review]
Add 3 assertions, whose purpose is to silence covreity

r=wtc.   This patch has some gratuitous changes
(combining variable declaration and variable assignment
into variable declaration with initializer).  These
changes are a matter of personal preference but make the
patch harder to review.

Comment 9

[Alexei Volkov]

Comment on attachment 222451 [details] [diff] [review]
Add 3 assertions, whose purpose is to silence covreity

r=alexei

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Checking in derenc.c; new revision: 1.2.28.1; previous revision: 1.2
Checking in derenc.c; new revision: 1.3; previous revision: 1.2

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

CIDs 495 496 497