Last Comment Bug 353543 - valgrind uninitialized memory read in nssPKIObjectCollection_AddInstances
: valgrind uninitialized memory read in nssPKIObjectCollection_AddInstances
Status: RESOLVED FIXED
: valgrind
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.11.3
: x86 Linux
: P4 minor (vote)
: 3.12
Assigned To: Nelson Bolyard (seldom reads bugmail)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-09-20 13:08 PDT by David Baron :dbaron: ⌚️UTC-10 (vacation, returning December 19)
Modified: 2008-01-21 15:22 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
untested patch v1 (1.05 KB, patch)
2007-12-29 23:46 PST, Nelson Bolyard (seldom reads bugmail)
wtc: review+
Details | Diff | Splinter Review

Description David Baron :dbaron: ⌚️UTC-10 (vacation, returning December 19) 2006-09-20 13:08:56 PDT
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] 2007-12-28 09:27:57 PST
Is this fixed on the NSS 3.12 branch being used by the trunk?
Comment 2 Nelson Bolyard (seldom reads bugmail) 2007-12-29 23:44:38 PST
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) 2007-12-29 23:46:51 PST
Created attachment 294878 [details] [diff] [review]
untested patch v1

Wan-Teh, please review
Comment 4 Wan-Teh Chang 2008-01-02 10:15:30 PST
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) 2008-01-21 15:20:41 PST
Checking in lib/pki/pkibase.c;
new revision: 1.30; previous revision: 1.29
Comment 6 Nelson Bolyard (seldom reads bugmail) 2008-01-21 15:22:31 PST
(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.

Note You need to log in before you can comment on or make changes to this bug.