Closed Bug 372449 Opened 18 years ago Closed 13 years ago

valgrind errors in our tests suites.

Categories

(NSS :: Test, defect)

x86
Windows XP
defect
Not set
trivial

Tracking

(Not tracked)

RESOLVED INCOMPLETE

People

(Reporter: rrelyea, Assigned: rrelyea)

Details

(Keywords: valgrind)

valgrind is a general tool for investigation programs at run time. It works by creating a virtual machine in which to run the code under investigation, then instrumenting the code with various hooks to evaluate call coverage, memory usage, and even race condition checking. The tool is very platform and processor specific, namely it works well on linux x86 and amd64 and fairly well on ppc and ppc 64 platforms. Experimental support for x86/NetBSD and x86/FreeBSd. I've run valgrind on linux/x86 against our all.sh scripts and found three errors: Error 1: In blapitest.c ==24467== Invalid read of size 1 ==24467== at 0x804F9E0: atob (blapitest.c:267) ==24467== by 0x8050554: setupIO (blapitest.c:903) ==24467== by 0x8055503: load_file_data (blapitest.c:2666) ==24467== by 0x805583E: get_params (blapitest.c:2743) ==24467== by 0x8055D78: blapi_selftest (blapitest.c:2907) ==24467== by 0x80565F2: main (blapitest.c:3269) ==24467== Address 0x4064F56 is 0 bytes after a block of size 30 alloc'd ==24467== at 0x40053C0: malloc (vg_replace_malloc.c:149) ==24467== by 0x4032316: PR_Malloc (prmem.c:467) ==24467== by 0x80FDDE9: PORT_Alloc (secport.c:113) ==24467== by 0x80FC582: SECITEM_AllocItem (secitem.c:77) ==24467== by 0x805890C: SECU_FileToItem (secutil.c:593) ==24467== by 0x8050469: setupIO (blapitest.c:881) ==24467== by 0x8055503: load_file_data (blapitest.c:2666) ==24467== by 0x805583E: get_params (blapitest.c:2743) ==24467== by 0x8055D78: blapi_selftest (blapitest.c:2907) ==24467== by 0x80565F2: main (blapitest.c:3269) The offending line is: len = (strcmp(&ascii->data[ascii->len-2],"\r\n")) ? ascii->len : ascii->len-2; We are reading passed the end of ascii->len to fetch the NULL. The correct code should be: len = (strncmp(&ascii->data[ascii->len-2],"\r\n",2)) ? ascii->len : ascii->len-2; Error 2: In blapitest.c ==24757== Conditional jump or move depends on uninitialised value(s) ==24757== at 0x80FC842: SECITEM_CompareItem (secitem.c:165) ==24757== by 0x80558ED: verify_self_test (blapitest.c:2780) ==24757== by 0x8055FB4: blapi_selftest (blapitest.c:2992) ==24757== by 0x80565F2: main (blapitest.c:3269) In this case the offending code is: int res; char *modestr = mode_strings[mode]; res = SECITEM_CompareItem(&result->pBuf, &cmp->buf); if (is_sigCipher(mode)) { if (forward) { if (res == 0) { printf("Signature self-test for %s passed.\n", modestr); } else { printf("Signature self-test for %s failed!\n", modestr); } } else { if (sigstatus == SECSuccess) { printf("Verification self-test for %s passed.\n", modestr); } else { printf("Verification self-test for %s failed!\n", modestr); } } return sigstatus; } else if (is_hashCipher(mode)) { The bad reference comes with we are verifying a dsa signature. In this case result->pBuf is never initialized, but we also never use the result. This is not really an error (though could have bad things happen if result->pBuf is never allocated). We can silence the warning and protect against the null case with the following code. int needcmp = !is_sigCipher(mode) || forward; res = needcmp && SECITEM_CompareItem(&result->pBuf, &cmp->buf); Error 3: in selfserv.c ==29670== Conditional jump or move depends on uninitialised value(s) ==29670== at 0x805155C: handle_connection (selfserv.c:1187) ==29670== by 0x80504C3: jobLoop (selfserv.c:517) ==29670== by 0x8050390: thread_wrapper (selfserv.c:486) ==29670== by 0x404AE3C: _pt_root (ptthread.c:220) ==29670== by 0x471FA2DA: start_thread (in /lib/libpthread-2.5.90.so) ==29670== by 0x47128D0D: clone (in /lib/libc-2.5.90.so) The offending code is: char buf[10240]; . {lots of if code deleted } . if (strncmp(buf, stopCmd, sizeof stopCmd - 1) == 0) { It is possible for buf to get to the strncmp uninitialized. Since buf is a stack variable it's possible, but exceedingly unlikely, that it could incorrectly match stopCmd. The simple fix for this is to initialize buf[0] = 0; at the top of the function. The main reason for fixing these issues in the test programs is to keep noise out of the error log when new NSS code is tested against valgrind. I suggest at least fixing Errors 1 and 3 since they are technically bugs. I would also like to fix Error 2. bob
Do we care about such errors in our test programs? In our libraries, yes we care. In our test programs?
By removing the errors in the test programs, I can use the tools (and all.sh) to detect errors in new code in our libraries. I have a patch for all three issues, which I shall attach to this bug. bob
Assignee: nobody → rrelyea
Keywords: valgrind
Bob, do you still have a patch for all three issues you're planning on attaching here? ;)
Bob's on vacation this month
Is this still actual?
Whiteboard: closeme INCO 2012-04-01
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → INCOMPLETE
Whiteboard: closeme INCO 2012-04-01
You need to log in before you can comment on or make changes to this bug.