166741 - Variables may be used uninitialized in devutil.c

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Wan-Teh Chang]

When compiling NSS under gcc on Linux, we get the
following warnings in devutil.c:

devutil.c: In function `find_objects_in_array':
devutil.c:1059: warning: `oi' might be used uninitialized in this function
devutil.c:1061: warning: `match' might be used uninitialized in this function

The first warning is benign, but the second warning
is an error.

(This bug refers to rev. 1.15 of devutil.c.)

Comment 1

[Ian McGreer]

Attachment: silence warnings

It wasn't an error, the value of match would never be checked unless the
previous loop terminated early, in which case match would have been set.  But I
agree that is non-obvious :)

Comment 2

[Wan-Teh Chang]

Here is the code in question:

        for (i=0; i<otlen; i++) {
            for (j=0; j<obj->numAttributes; j++) {
                if (ot[i].type == obj->attributes[j].type) {
                    if (ot[i].ulValueLen == obj->attributes[j].ulValueLen &&
                        nsslibc_memequal(ot[i].pValue,
                                         obj->attributes[j].pValue,
                                         ot[i].ulValueLen, NULL))
                    {
                        match = PR_TRUE;
                    } else {
                        match = PR_FALSE;
                    }
                    break;
                }
            }
            if (j == obj->numAttributes || !match) {
                break;
            }
        }
        if (match) {

'match' is used in two places.  In the first place, I agree
'match' can't be used uninitialized.  In the second place,
I think 'match' may be used uninitialized.  For example, if
we break out of the outer for loop because j == obj->numAttributes,
'match' won't be set.

I have another question: do you still want to break out of the
inner for loop if you set 'match' to PR_FALSE?

Comment 3

[Ian McGreer]

Okay, agreed.

Yes, I want to break out of the loop.  I am looking for a particular type in the
array.  If the type is found, I'm done.  'match' indicates whether the type had
the same value between the two arrays.

Comment 4

[Ian McGreer]

Attachment: based on wtc's review

Comment 5

[Wan-Teh Chang]

Comment on attachment 98030 [details] [diff] [review]
based on wtc's review

r=wtc.

Comment 6

[Ian McGreer]

checked in.

Comment 7

[Wan-Teh Chang]

The patch caused bug 167532.  I am reopening the bug.

Comment 8

[Wan-Teh Chang]

Attachment: Incremental patch

This patch fixes bug 167532.

Comment 9

[Ian McGreer]

Comment on attachment 98548 [details] [diff] [review]
Incremental patch


Patch is correct.

That code could definitely be made more readable.  I overlooked that the goal
wasn't to match a single attribute, but a whole set.  It is possible for match
to be true, and then changed to false within the loop.

Comment 10

[Ian McGreer]

Attachment: yet another patch

After further discussion with Wan-Teh, it appeared there wer still bugs in the
above two patches.  This patch tries to clean the code up, so it is more clear
what is going on.

Comment 11

[Wan-Teh Chang]

Comment on attachment 98610 [details] [diff] [review]
yet another patch

r=wtc.

Could you add a comment describing what the
find_objects_in_array function does?

Comment 12

[Ian McGreer]

patch checked in, marking fixed.