225808 - CERTCertificate.nsCertType is unsigned int but is passed to PR_AtomicSet

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P3)

Description

[Wan-Teh Chang]

The nsCertType field of CERTCertificate is an unsigned int
but its address (unsigned int *) is passed to PR_AtomicSet,
which expects a PRInt32 *.

This causes a compiler warning and is a bug where
sizeof(int) is not 4 bytes (32 bits).  (None of the platforms
we support today defines int to be 16 bit or 64 bit.)

The fix is to either declare nsCertType to be a PRInt32
(which may require additional changes to avoid the
signed/unsigned compiler warnings) or use a lock to
protect the update of nsCertType.

Strictly speaking, since we are only protecting the update
but not the read of nsCertType, it is not thread safe.
That should be the subject of another bug.  This bug is
only about the (potential) "unsigned int" vs. "PRInt32"
size mismatch.

Comment 1

[Wan-Teh Chang]

Another option is to declare nsCertType to be PRUint32
and cast its address to (PRInt32 *) for PR_AtomicSet.
It is the size of nsCertType that's important to
PR_AtomicSet.  Of course, we should consider this only
if changing from unsigned int to PRInt32 causes
signed/unsigned compiler warnings or bugs.

Comment 2

[Wan-Teh Chang]

Attachment: Proposed patch

On second thought, I think it is safer to just declare
cert->nsCertType as PRUint32 so that we don't need to
worry about any bugs that might arise from the signedness
change.  To eliminate the compiler warning, the (PRInt32 *)
cast is necessary, and I added an assertion to make sure
that cast is safe.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Maybe even slightly better than 
+    /* Assert that it is safe to cast &cert->nsCertType to "PRInt32 *" */
+    PORT_Assert(sizeof(cert->nsCertType) == 4);
would be
+    /* Assert that it is safe to cast &cert->nsCertType to "PRInt32 *" */
+    PORT_Assert(sizeof(cert->nsCertType) == sizeof(PRInt32));

Comment 4

[Wan-Teh Chang]

Attachment: Proposed patch v1.1

This patch implements Nelson's suggestion in comment 3.

Comment 5

[Julien Pierre]

Comment on attachment 136674 [details] [diff] [review]
Proposed patch v1.1

Wan-Teh,

Changing nsCertType from int to PRInt32 implies breaking the binary
compatibility for shared libraries on those platforms where sizeof(int) !=
sizeof(PRInt32). While the fix is good, I feel that this one belongs in a major
NSS release, not a minor release.

I do realize now (unfortunately not when I added that call) that PR_AtomicSet
currently misbehaves on those platforms.

One fix would be to use conditional compilation.

It may be possible to do something like

#if sizeof(int) != sizeof(PRInt32)

PR_Lock(...)

#else

PR_AtomicSet(..)
#endif

This is not very clean, but it would not break binary compatibility.

I think the locking however would be quite a performance hit, so we may never
want to do it at all.

This could be a case where we need another atomicset function that needs to
work on int*. Maybe we could have some conditional compilation like this :

#if sizeof(int) == sizeof(PRInt32)

PR_AtomicSet(...)

#elif sizeof(int) == sizeof(PRInt16)

PR_AtomicSet16(...)

#elif sizeof(int) == sizeof(PRInt64)

PR_AtomicSet64(...)

#elif

#error "This code need to be fixed for your platform"

#endif

Or the macro could live in an NSPR header and define a PR_AtomicSetInt ...

You have identified another problem with the thread-safety of this code. As I
remember, this fix I coded was a compromise. Strictly speaking, it wasn't
completely thread-safe, but it was much better than what was there before, and
effectively masked the problem of multiple threads updating nsCertType in our
SSL server tests. If you have a better idea of how to fix the code that perhaps
doesn't involve using PR_AtomicSet, you are welcome to propose one.

Comment 6

[Wan-Teh Chang]

The only platform that we have ever supported where int is
not 32-bit is WIN16.  So, binary compatibility is not an issue.

To make the code thread-safe we must use a lock, and there will
be many more places (where we *read* or update cert->nsCertType)
that need to be modified.  The thread-safety issue should be
the subject of another bug.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 136674 [details] [diff] [review]
Proposed patch v1.1

Given that we support no platforms with ints >32 bits, I think this patch is
fine.

Comment 8

[Wan-Teh Chang]

Attachment: NSPR's definition of PR_BYTES_PER_INT

This file shows that _win16.cfg is the only file
that doesn't define PR_BYTES_PER_INT to be 4.

Comment 9

[Wan-Teh Chang]

Patch checked in.

Checking in certdb.c;
/cvsroot/mozilla/security/nss/lib/certdb/certdb.c,v  <--  certdb.c
new revision: 1.63; previous revision: 1.62
done
Checking in certt.h;
/cvsroot/mozilla/security/nss/lib/certdb/certt.h,v  <--  certt.h
new revision: 1.26; previous revision: 1.25
done

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Setting priorities on unprioritized bugs resolved fixed for NSS 3.10.
Setting this to P3, since it was motivated only as warning reduction.