Closed
Bug 372449
Opened 18 years ago
Closed 13 years ago
valgrind errors in our tests suites.
Categories
(NSS :: Test, defect)
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
Comment 1•18 years ago
|
||
Do we care about such errors in our test programs?
In our libraries, yes we care.
In our test programs?
Assignee | ||
Comment 2•18 years ago
|
||
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
Comment 3•17 years ago
|
||
Bob, do you still have a patch for all three issues you're planning on attaching here? ;)
Comment 4•17 years ago
|
||
Bob's on vacation this month
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.
Description
•