Last Comment Bug 317856 - uninitialized variables in strsclnt.c
: uninitialized variables in strsclnt.c
Status: RESOLVED FIXED
:
Product: NSS
Classification: Components
Component: Tools (show other bugs)
: 3.11
: x86 Linux
: P2 normal (vote)
: 3.11.1
Assigned To: Julien Pierre
: Jason Reid
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-11-26 05:14 PST by Wolfgang Rosenauer [:wolfiR]
Modified: 2006-01-20 14:56 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
fix as checked in to the tip (1.82 KB, patch)
2006-01-19 16:43 PST, Julien Pierre
wtc: review-
Details | Diff | Review
Patch v2 (706 bytes, patch)
2006-01-19 18:16 PST, Wan-Teh Chang
no flags Details | Diff | Review
Patch v2 (corrected) (708 bytes, patch)
2006-01-19 18:19 PST, Wan-Teh Chang
julien.pierre: review+
Details | Diff | Review

Description Wolfgang Rosenauer [:wolfiR] 2005-11-26 05:14:48 PST
strsclnt.c: In function 'launch_thread':
strsclnt.c:500: warning: 'i' is used uninitialized in this function
strsclnt.c: In function 'do_connects':
strsclnt.c:846: warning: 'now' is used uninitialized in this function

The first one is just used for a printf and not critical. I'm not sure for the second:
PRTime now;
PR_Lock(threadLock);
lastConnectSuccess = PR_MAX(now, lastConnectSuccess);

This seems broken to me at first glance.
Comment 1 Wan-Teh Chang 2005-11-26 06:57:44 PST
Wolfgang, thanks for the bug report.

These bugs were introduced by the fix for bug 292151
in NSS 3.11.  The fix should be:
1. remove 'i'.  It probably should be replaced by 'numUsed-1'
   or 'numUsed' in the PRINTF statement.  The message
   "strsclnt: Launched thread in slot %d \n" may need to be
   updated to describe the new code (if the new code no longer
   uses slots).
2. initialize 'now' with PR_Now().
Comment 2 Julien Pierre 2006-01-19 16:43:33 PST
Created attachment 209037 [details] [diff] [review]
fix as checked in to the tip

These are the fixes suggested by Wan-Teh, and they are correct.
Checking in strsclnt.c;
/cvsroot/mozilla/security/nss/cmd/strsclnt/strsclnt.c,v  <--  strsclnt.c
new revision: 1.45; previous revision: 1.44
done
Comment 3 Julien Pierre 2006-01-19 16:44:52 PST
Changing priority. This is P2, not P1 . Target is 3.12 .
Comment 4 Wan-Teh Chang 2006-01-19 17:28:23 PST
Julien, strsclnt is used in our test suite to determine
whether a build passes QA.  Also, we usually fix all the
compiler warnings in the libraries and the important
commands, but we didn't do that before the 3.11 release.
So your fix should be checked into the NSS_3_11_BRANCH
for 3.11.1.
Comment 5 Wan-Teh Chang 2006-01-19 17:30:34 PST
I meant to say that
  we usually fix all the compiler warnings in the
  libraries and the important commands *before each
  minor release*, ...
Comment 6 Julien Pierre 2006-01-19 17:33:17 PST
OK, I checked it in on the NSS_3_11_BRANCH :

Checking in strsclnt.c;
/cvsroot/mozilla/security/nss/cmd/strsclnt/strsclnt.c,v  <--  strsclnt.c
new revision: 1.43.2.1; previous revision: 1.43
done
Comment 7 Wan-Teh Chang 2006-01-19 18:08:00 PST
Comment on attachment 209037 [details] [diff] [review]
fix as checked in to the tip

Julien,

I found that numUsed should only be used while holding
threadLock, so I suggest a different fix for "i".

We should keep "i".  Do not initialize it.

Set "i" here:

+     i = numUsed;
      slot = &threads[numUsed++];

And continue to use "i" in the PRINTF statement, which is
executed after we unlock threadLock.

      slot->inUse   = 1;
      PR_Unlock(threadLock);
      PRINTF("strsclnt: Launched thread in slot %d \n", i);
                                                       ^^^
Comment 8 Wan-Teh Chang 2006-01-19 18:16:54 PST
Created attachment 209044 [details] [diff] [review]
Patch v2
Comment 9 Wan-Teh Chang 2006-01-19 18:19:06 PST
Created attachment 209045 [details] [diff] [review]
Patch v2 (corrected)
Comment 10 Nelson Bolyard (seldom reads bugmail) 2006-01-19 22:52:59 PST
reopened in light of most recent comments.
Comment 11 Julien Pierre 2006-01-20 14:56:01 PST
Comment on attachment 209045 [details] [diff] [review]
Patch v2 (corrected)

Indeed, thanks for catching this. I have checked the corrections to both the tip and the 3.11 branch.

Checking in strsclnt.c;
/cvsroot/mozilla/security/nss/cmd/strsclnt/strsclnt.c,v  <--  strsclnt.c
new revision: 1.46; previous revision: 1.45


Checking in strsclnt.c;
/cvsroot/mozilla/security/nss/cmd/strsclnt/strsclnt.c,v  <--  strsclnt.c
new revision: 1.43.2.2; previous revision: 1.43.2.1
done

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