333389 - sftk_NewAttribute should not crash when so is NULL [@ sftk_NewAttribute]

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P3)

Description

[timeless]

Code that uses PORT_Assert expects to die. if it doesn't, it'll crash later.

Unfortunately, PR_ASSERT is only fatal in some builds. If you want to be always fatal, you're supposed to use PR_Assert directly.

See http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/softoken/pkcs11u.c&rev=1.70&mark=64,67-69,83#58

for a case where coverity complained

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

PORT_Assert is only fatal in DEBUG builds, by design.
I think the essence of this bug report is that, in non-DEBUG builds,
sftk_NewAttribute() crashes when so is NULL, rather failing gracefully.  
Is that assessment of this bug correct?

Comment 2

[timeless]

yes. i kinda presumed that the fault was in PORT_Assert not doing as the code expected. but if PORT_Assert really wants the behavior it has, then i'll gladly morph this bug to what coverity originally implicated instead of what i divined:

if sftk_narrowToSessionObject returns null we crash.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Is this crash actually being seen, at all? 
If not, this is P3 at most.

Comment 4

[timeless]

if i have steps or talkback bits, i'd include them. the bugs i'm filing w/ coverity keywords are direct from scan.coverity.com w/ their analysis replaced by my hand written bonsai links.

i really don't care when you fix bugs. but in general the coverity bugs i file are simple, and i am semi actively recruiting newbies to work on such bugs, so don't be surprised if someone else posts a patch before you do (especially the longer you take to get to the bug).

Comment 5

[Alexei Volkov]

Attachment: return null if SFTKSessionObject is NULL

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 219802 [details] [diff] [review]
return null if SFTKSessionObject is NULL

r=nelson

Comment 7

[Alexei Volkov]

tip:
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11u.c,v  <--  pkcs11u.c
new revision: 1.72; previous revision: 1.71

3.11 branch:
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11u.c,v  <--  pkcs11u.c
new revision: 1.69.2.3; previous revision: 1.69.2.2

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Please also check this in on the NSS_3_11_1_BRANCH

Comment 9

[Alexei Volkov]

3.11.1:
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11u.c,v  <--  pkcs11u.c
new revision: 1.69.2.2.2.1; previous revision: 1.69.2.2

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

CID 228