303494 - SEC_LookupCrls(.., .., SEC_KRL_TYPE) returns CRLs, and SEC_LookupCrls(.., .., SEC_CRL_TYPE) returns nothing

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Julien Pierre]

It's been reported to us that this API returns CRLs when a KRL was requested,
and nothing when CRLs are requested .
This happens only on Windows, when NSS is built with VC6 optimized . A debug
build works fine .

The problem is easily reproducible with crlutil -L -t 0 / crlutil -L -t 1 .

SEC_LookupCrls does a token search, but I determined that softoken isn't at
fault, because the bug still happens when I use a debug softokn3.dll with an
optimized nss3.dll . Conversely, when I use a debug build, the bug happens once
I replace nss3.dll with an optimized version . So, the bug is in the code from
libnss .

I have been looking for uninitialized variables all afternoon without success,
using dbx on Solaris and purify on Windows . No luck. Building in the OPT.OBJD
configuration also did not help . I managed to get symbols in my optimized
build, but the resulting code is extremely hard to trace (the program counter
jumps in non-linear order of source lines !).

I changed the optimization level from -O2 to -O1 . At that point, SEC_LookupCrls
started returned the CRL for *both* the SEC_CRL_TYPE and SEC_KRL_TYPE queries.

I'm nearly convinced this is a code generation bug in VC6 . I'm using VC6 SP6 FWIW .

Comment 1

[Julien Pierre]

This is actually a bug in PK11_LookupCrls, but it is only triggered by the
Microsoft VC6 optimizer .

Here is the disassembly of the incriminating area. This is the output of the
compiler with -O1, in the case where CRLs are always returned , regardless of
the input argument :

249:  /*
250:   * Return a list of all the CRLs .
251:   * CRLs are allocated in the list's arena.
252:   */
253:  SECStatus
254:  PK11_LookupCrls(CERTCrlHeadNode *nodes, int type, void *wincx) {
002421BD 55                   push        ebp
002421BE 8B EC                mov         ebp,esp
002421C0 83 EC 2C             sub         esp,2Ch
255:      pk11TraverseSlot creater;
256:      CK_ATTRIBUTE theTemplate[2];
257:      CK_ATTRIBUTE *attrs;
258:      CK_OBJECT_CLASS certClass = CKO_NETSCAPE_CRL;
259:
260:      attrs = theTemplate;
261:      PK11_SETATTRS(attrs, CKA_CLASS, &certClass, sizeof(certClass)); attrs++;
002421C3 83 65 D4 00          and         dword ptr [theTemplate],0
262:      if (type != -1) {
002421C7 83 7D 0C FF          cmp         dword ptr [type],0FFFFFFFFh
002421CB 8D 45 FC             lea         eax,[certClass]
002421CE C7 45 FC 51 43 53 CE mov         dword ptr [certClass],0CE534351h
002421D5 89 45 D8             mov         dword ptr [ebp-28h],eax
002421D8 C7 45 DC 04 00 00 00 mov         dword ptr [ebp-24h],4
002421DF 8D 45 E0             lea         eax,[ebp-20h]
002421E2 74 17                je          PK11_LookupCrls+3Eh (002421fb)
263:      CK_BBOOL isKrl = (CK_BBOOL) (type == SEC_KRL_TYPE);
264:          PK11_SETATTRS(attrs, CKA_NETSCAPE_KRL, &isKrl, sizeof(isKrl));
attrs++;
002421E4 8D 45 0F             lea         eax,[isKrl]
002421E7 C7 45 E0 58 43 53 CE mov         dword ptr [ebp-20h],0CE534358h
002421EE 89 45 E4             mov         dword ptr [ebp-1Ch],eax
002421F1 C7 45 E8 01 00 00 00 mov         dword ptr [ebp-18h],1
002421F8 8D 45 EC             lea         eax,[creater]
265:      }

The problem is that isKrl is a temporary variable, declared within the if (type
!= -1) { } block . But we are taking its address and setting it in the template.
Somehow, the optimizer decided not to initialize that variable which was
dropping out of stack anyway ;)

I guess in the -O2 case, the stack offset probably gets reused by some other
variable later on.

The fix is simply to declare isKrl at the top of the function .

Comment 2

[Julien Pierre]

Attachment: Move isKrl variable to function scope

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 191658 [details] [diff] [review]
Move isKrl variable to function scope

r=nelson.bolyard

Comment 4

[Wan-Teh Chang]

Comment on attachment 191658 [details] [diff] [review]
Move isKrl variable to function scope

Good job. r=wtc.

Comment 5

[Julien Pierre]

I checked this in to the tip.

Checking in pk11nobj.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11nobj.c,v  <--  pk11nobj.c
new revision: 1.6; previous revision: 1.5
done

This bug has been around since 3.4 .

Comment 6

[Julien Pierre]

We had a customer request this for 3.10.2, so I checked this in to NSS_3_10_BRANCH .

Changing target to 3.10.2 .

Checking in pk11nobj.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11nobj.c,v  <--  pk11nobj.c
new revision: 1.4.8.1; previous revision: 1.4
done