regression: cert->emailAddr is never NULL

RESOLVED FIXED in 3.9

Status

P2
normal
RESOLVED FIXED
16 years ago
13 years ago

People

(Reporter: nelson, Assigned: nelson)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [3.8.2][3.7.8])

Attachments

(2 attachments, 1 obsolete attachment)

The fix for bug 152986 introduced a regression in NSS.

Prior to that fix, if a cert had no email address, cert->emailAddr would be
NULL.  There are numerous places in NSS that test that pointer for NULL to 
see if the cert has an email address, when then take different courses of 
action depending on whether the pointer is NULL or not.  This undoubtedly
is causing undesirable behavior, such as creating empty attributes on some
cert objects.

The new function cert_GetCertificateEmailAddresses differs from the function 
it replaced when the cert contains no email address.  The old function returned
NULL.  The new function returns a pointer to an empty string.  This appears to
be wrong. 

I recommend that cert_GetCertificateEmailAddresses be changed to return NULL
rather than a pointer to an empty string, AND that all places that test
cert->emailAddr be changed to test for either a NULL pointer or an empty string,
e.g. 

-   if (cert->emailAddr)
+   if (cert->emailAddr && cert->emailAddr[0])
(Assignee)

Comment 1

16 years ago
taking bug
Assignee: wtc → nelsonb
Severity: major → normal
Target Milestone: --- → 3.9
(Assignee)

Comment 2

16 years ago
Created attachment 126985 [details] [diff] [review]
return NULL instead of ptr to empty string.

This patch implements the first part of my propsed solution.  It will cause
the emailAddr pointer to be NULL instead of pointing to an empty string.

The second part of my proposed solution, adding testing for an empty string,
is suggested to detect any undesirable effects that have already taken place.
I will code a separate patch for that.

Comment 3

16 years ago
Comment on attachment 126985 [details] [diff] [review]
return NULL instead of ptr to empty string.

>     /* now copy superstring to cert's arena */
>     finalLen = (pBuf - addrBuf) + 1;
>-    pBuf = PORT_ArenaAlloc(cert->arena, finalLen);
>-    if (pBuf) {
>-    	PORT_Memcpy(pBuf, addrBuf, finalLen);

It seems that the last byte we are copying from
(addrBuf[finalLen-1]) is never set to 0.  Am I
right?

Comment 4

16 years ago
Comment on attachment 126985 [details] [diff] [review]
return NULL instead of ptr to empty string.

r=wtc.

Please ignore my previous comment.  addrBuf is
allocated with PORT_ArenaZAlloc, so all the bytes
are initialized to 0.  Sorry about my confusion.
Attachment #126985 - Flags: review+
(Assignee)

Comment 5

16 years ago
Above patch checked in.

Comment 6

16 years ago
Comment on attachment 126985 [details] [diff] [review]
return NULL instead of ptr to empty string.

I checked in this patch on the NSS_3_8_BRANCH (NSS 3.8.2)
and NSS_3_7_BRANCH (3.7.8) because this is a regression
introduced in NSS 3.7.

Updated

16 years ago
Whiteboard: [3.8.2][3.7.8]

Updated

15 years ago
Priority: -- → P2
(Assignee)

Comment 7

15 years ago
Created attachment 132239 [details] [diff] [review]
patch part2

This patch supplements the previous patch (which is already checked in).
It detects empty emailAddr strings in certs and in the databases, and 
treats them the same as NULL emailAddr string pointers.  all.sh passes 
with this patch.
(Assignee)

Updated

15 years ago
Attachment #132239 - Flags: review?(wchang0222)
(Assignee)

Updated

15 years ago
Status: NEW → ASSIGNED

Comment 8

15 years ago
Comment on attachment 132239 [details] [diff] [review]
patch part2

1. In cmd/lib/seccnames.c, function sec_CollectCertNamesAndTrust,
the new local variable 'addr' should be declared as 'char *', not
'char'.

2. I disagree with some of the changes.  I will talk to you about
them offline.
Attachment #132239 - Flags: review?(wchang0222) → review-
(Assignee)

Comment 9

15 years ago
Subsequent to comment 8 above, cmd/lib/seccnames.c was removed from the tree,
so there is no need to change it further.

Wan-teh's other comments re patch part 2 above were:

a) Callers of CERT_GetFirtsEmailAddress and CERT_GetNextEmailAddress ought not
   to have to check for both a NULL pointer and a pointer to an empty string.
   The function should never return a pointer to an empty string.

b) Two essentially identical functions: 
      nsslowcert_FixupEmailAddr() in lib/softoken/lowcert.c and
      CERT_FixupEmailAddr         in lib/certdb/certdb.c
   take string pointers and return pointers to strdup'ed strings that 
   have been "fixed up" (read: downshifted).  These functions ought not
   to return NULL unless (a) they were passed NULL, or (b) strdup failed.

Since the time of those comments, code that was common to cmd/vfyutil and 
cmd/vfychain has been factored out and moved to cmd/lib.  The next patch
(part 2 v2) will address these points.
(Assignee)

Comment 10

15 years ago
Created attachment 135295 [details] [diff] [review]
Patch part 2 - v2

I think this patch addresses the same issues as attachment 132239 [details] [diff] [review] above,
but also incorporates Wan-Teh's review feedback.
Attachment #132239 - Attachment is obsolete: true
(Assignee)

Comment 11

15 years ago
Comment on attachment 135295 [details] [diff] [review]
Patch part 2 - v2

I'd be interested in seeing what impact, if any, this patch has on bug 224663
Attachment #135295 - Flags: review?(wchang0222)

Comment 12

15 years ago
Comment on attachment 135295 [details] [diff] [review]
Patch part 2 - v2

r=wtc.	This patch is good.  I have a question about dbck.c.
Since we are not using dbck.c right now, the question is low
priority.  I will ask Nelson the question offline.

Bob, could you review this patch, too?
Attachment #135295 - Flags: superreview?(rrelyea0264)
Attachment #135295 - Flags: review?(wchang0222)
Attachment #135295 - Flags: review+

Updated

15 years ago
Attachment #135295 - Flags: superreview?(rrelyea0264) → superreview+
(Assignee)

Comment 13

15 years ago
/cvsroot/mozilla/security/nss/cmd/dbck/dbck.c,v  <--  dbck.c
new revision: 1.5; previous revision: 1.4

/cvsroot/mozilla/security/nss/cmd/signtool/util.c,v  <--  util.c
new revision: 1.16; previous revision: 1.15

/cvsroot/mozilla/security/nss/lib/certdb/certdb.c,v  <--  certdb.c
new revision: 1.56; previous revision: 1.55

/cvsroot/mozilla/security/nss/lib/certdb/stanpcertdb.c,v  <--  stanpcertdb.c
new revision: 1.60; previous revision: 1.59

/cvsroot/mozilla/security/nss/lib/pkcs7/p7decode.c,v  <--  p7decode.c
new revision: 1.14; previous revision: 1.13

/cvsroot/mozilla/security/nss/lib/pki/certificate.c,v  <--  certificate.c
new revision: 1.49; previous revision: 1.48

/cvsroot/mozilla/security/nss/lib/pki/pki3hack.c,v  <--  pki3hack.c
new revision: 1.79; previous revision: 1.78

/cvsroot/mozilla/security/nss/lib/smime/cmssiginfo.c,v  <--  cmssiginfo.c
new revision: 1.19; previous revision: 1.18

/cvsroot/mozilla/security/nss/lib/softoken/pkcs11u.c,v  <--  pkcs11u.c
new revision: 1.52; previous revision: 1.51
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
(Assignee)

Comment 14

15 years ago
Also
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11cert.c,v  <--  pk11cert.c
new revision: 1.127; previous revision: 1.126

Comment 15

13 years ago
according to my uninformed reading, this resulted in bug 224663
Blocks: 224663
You need to log in before you can comment on or make changes to this bug.