317856 - uninitialized variables in strsclnt.c

Status: RESOLVED FIXED

(NSS :: Tools, defect, P2)

Description

[Wolfgang Rosenauer [:wolfiR]]

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]

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]

Attachment: 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]

Changing priority. This is P2, not P1 . Target is 3.12 .

Comment 4

[Wan-Teh Chang]

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]

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]

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]

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]

Attachment: Patch v2

Comment 9

[Wan-Teh Chang]

Attachment: Patch v2 (corrected)

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

reopened in light of most recent comments.

Comment 11

[Julien Pierre]

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