353543 - valgrind uninitialized memory read in nssPKIObjectCollection_AddInstances

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P4)

Description

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

I did a valgrind run on the MOZILLA_1_8_BRANCH as of yesterday, and hit the following warning:

==15825== Thread 8:
==15825== Conditional jump or move depends on uninitialised value(s)
==15825==    at 0x12351C09: nssPKIObjectCollection_AddInstances (pkibase.c:880)
==15825==    by 0x1234D2A7: nssTrustDomain_FindCertificateByIssuerAndSerialNumber (trustdomain.c:823)
==15825==    by 0x1234D3D5: nssTrustDomain_FindCertificateByEncodedCertificate (trustdomain.c:879)
==15825==    by 0x1234D40D: NSSTrustDomain_FindCertificateByEncodedCertificate (trustdomain.c:893)
==15825==    by 0x1233279F: __CERT_NewTempCertificate (stanpcertdb.c:244)
==15825==    by 0x1219D8A1: ssl3_HandleCertificate (ssl3con.c:6969)
==15825==    by 0x1219F24F: ssl3_HandleHandshakeMessage (ssl3con.c:7664)
==15825==    by 0x1219F638: ssl3_HandleHandshake (ssl3con.c:7780)
==15825==    by 0x121A00E0: ssl3_HandleRecord (ssl3con.c:8043)
==15825==    by 0x121A12CD: ssl3_GatherCompleteHandshake (ssl3gthr.c:206)
==15825==    by 0x121A3E11: ssl_GatherRecord1stHandshake (sslcon.c:1258)
==15825==    by 0x121AC1D6: ssl_Do1stHandshake (sslsecur.c:149)
==15825==    by 0x121AE0E8: ssl_SecureSend (sslsecur.c:1090)
==15825==    by 0x121AE24B: ssl_SecureWrite (sslsecur.c:1128)
==15825==    by 0x121B448C: ssl_Write (sslsock.c:1413)
==15825==    by 0x11EAB641: nsSSLThread::Run() (nsSSLThread.cpp:913)
==15825==    by 0x11EAA9CB: nsPSMBackgroundThread::nsThreadRunner(void*) (nsPSMBackgroundThread.cpp:44)
==15825==    by 0x526A486: _pt_root (ptthread.c:220)
==15825==    by 0x3C6A606324: start_thread (pthread_create.c:287)
==15825==    by 0x3C695CBBAC: clone (in /lib64/libc-2.4.so)

It looks like this function accepts both null-terminated and counted arrays, and the warning is because it's checking for null-termination before it checks the count (thus reading past the end of a counted array), rather than checking the count before checking the null-termination.  This is mostly just a warning, although it could be a problem if the array passed in happens to be at the very edge of one of the segments of heap addresses allocated to the process.

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Is this fixed on the NSS 3.12 branch being used by the trunk?

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

At this time, the "NSS 3.12 branch" is the trunk.
The code in nssPKIObjectCollection_AddInstances is unchanged since 
this bug was reported.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Attachment: untested patch v1

Wan-Teh, please review

Comment 4

[Wan-Teh Chang]

Comment on attachment 294878 [details] [diff] [review]
untested patch v1

r=wtc.

Did you use a while loop instead of a for loop so that you could avoid having to wrap a
long line?

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Checking in lib/pki/pkibase.c;
new revision: 1.30; previous revision: 1.29

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #4)

> Did you use a while loop instead of a for loop so that you could avoid having
> to wrap a long line?

Yes.