918697 - bltest leaks file descriptors and does not check for file open errors

Status: NEW

(NSS :: Test, defect, P2)

Description

[Julien Pierre]

The above test fails on HP-UX 32-bit OPT, DBG and 64-bit DBG.

HP-UX 64-bit OPT fails to build properly (crash in shlibsign, bug 703812 ) . That also appears to be a problem related to DSA code.

NSS 3.13.1 builds and runs fine on the same machine with the same HP compiler.

I have tried building freebl without the assembly optimizations, but this does not make any difference for this test.

Each bltest iteration in the DSA Verify test fails fails with a signature error :

bltest -T -m dsa -V -d /export/314/mozilla/security/nss/tests/../cmd/bltest -1 7 -2 7
bltest: ERR -8182 (Peer's certificate has an invalid signature.) at line 2277.

And this is the final report :

cipher.sh: #30: DSA Verify (Failed in/out offset pairs:  [0:0][0:1][0:2][0:3][0:4][0:5][0:6][0:7][1:0][1:1][1:2][1:3][1:4][1:5][1:6][1:7][2:0][2:1][2:2][2:3][2:4]
[2:5][2:6][2:7][3:0][3:1][3:2][3:3][3:4][3:5][3:6][3:7][4:0][4:1][4:2][4:3][4:4][4:5][4:6][4:7][5:0][5:1][5:2][5:3][5:4][5:5][5:6][5:7][6:0][6:1][6:2][6:3][6:4][6
:5][6:6][6:7][7:0][7:1][7:2][7:3][7:4][7:5][7:6][7:7]) - FAILED

It looks like this test has changed significantly in NSS 3.14 due to DSA-2 .

Running the old bltest program from NSS 3.13.1 against NSS 3.14.3 libs (minus the assembly code, but I'm not sure that makes any diference) works fine.

Comment 1

[Julien Pierre]

This test also fails on 64-bit OPT build once the HP-UX assembly optimizations are disabled in freebl/Makefile . So, it really fails all the time on HP-UX regardless of 32/64 and OPT/DBG.

Comment 2

[Julien Pierre]

This is the stack of the failure when running 

bltest -T -m dsa -V -d /export/314/mozilla/security/nss/tests/../cmd/bltest -1 0 -2 0

on a 64-bit debug binary built with mpi_hp (assembly optimizations) :

Breakpoint 3, PR_SetError (code=-8182, osErr=0) at ../../../../pr/src/misc/prerror.c:25
25          PRThread *thread = PR_GetCurrentThread();
(gdb) where
#0  PR_SetError (code=-8182, osErr=0) at ../../../../pr/src/misc/prerror.c:25
#1  0x800003ffff6ffecc in PORT_SetError_Util (value=-8182) at secport.c:151
#2  0x800003ffff476cb8 in DSA_VerifyDigest (key=0x80000001000338a8, signature=0x800003ffff7f1a10, digest=0x800003ffff7f18a0) at dsa.c:612
#3  0x40000000000829d4 in DSA_VerifyDigest (key=0x80000001000338a8, signature=0x800003ffff7f1a10, digest=0x800003ffff7f18a0) at loader.c:244
#4  0x4000000000026118 in dsaOp (cipherInfo=0x800003ffff7f1860) at blapitest.c:2273
#5  0x400000000002760c in cipherDoOp (cipherInfo=0x800003ffff7f1860) at blapitest.c:2441
#6  0x400000000002aaa4 in blapi_selftest (modes=0x800003ffff7f16b8, numModes=1, inoff=0, outoff=0, encrypt=0, decrypt=1) at blapitest.c:3203
#7  0x400000000002c6ec in main (argc=11, argv=0x800003ffff7f1208) at blapitest.c:3724
(gdb) 

The problem occurs in dsa.c :


(gdb) list
605         CHECK_MPI_OK( mp_exptmod(&y, &u2, &p, &y) ); /* y = y**u2 mod p */
606         CHECK_MPI_OK(  mp_mulmod(&g, &y, &p, &v)  ); /* v = g * y mod p */
607         CHECK_MPI_OK(     mp_mod(&v, &q, &v)      ); /* v = v mod q     */
608         /*
609         ** Verification:  v == r'
610         */
611         if (mp_cmp(&v, &r_)) {
612             PORT_SetError(SEC_ERROR_BAD_SIGNATURE);
613             verified = SECFailure; /* Signature failed to verify. */
614         } else {
(gdb) p v
$1 = {sign = 0, alloc = 64, used = 4, dp = 0x8000000100036058}
(gdb) p r_
$2 = {sign = 0, alloc = 64, used = 4, dp = 0x8000000100036270}
(gdb) 

When rebuilding and not using mpi_hp, the failure is at the same place in dsa.c, but the values are different :
(gdb) p v
$1 = {sign = 0, alloc = 64, used = 4, dp = 0x80000001000360e8}
(gdb) p r_
$2 = {sign = 0, alloc = 64, used = 4, dp = 0x8000000100036300}
(gdb) where
#0  DSA_VerifyDigest (key=0x80000001000338a8, signature=0x800003ffff7f1a10, digest=0x800003ffff7f18a0) at dsa.c:612
#1  0x40000000000829d4 in DSA_VerifyDigest (key=0x80000001000338a8, signature=0x800003ffff7f1a10, digest=0x800003ffff7f18a0) at loader.c:244
#2  0x4000000000026118 in dsaOp (cipherInfo=0x800003ffff7f1860) at blapitest.c:2273
#3  0x400000000002760c in cipherDoOp (cipherInfo=0x800003ffff7f1860) at blapitest.c:2441
#4  0x400000000002aaa4 in blapi_selftest (modes=0x800003ffff7f16b8, numModes=1, inoff=0, outoff=0, encrypt=0, decrypt=1) at blapitest.c:3203
#5  0x400000000002c6ec in main (argc=11, argv=0x800003ffff7f1208) at blapitest.c:3724
(gdb)

Comment 3

[Julien Pierre]

Not sure if these help any more than the pointer values, which couldn't have been too helpful :

(gdb) p /x *v.dp
$10 = 0x2083ea467693e767
(gdb) p /x *r_.dp
$11 = 0x527d4cf6328738c8
(gdb) p /x *(long)r_.dp

It is always the 8th call to dsaOp that fails in this case.

Comment 4

[Julien Pierre]

Here is a printout of all the MPIs  on HP-UX with a breakpoint in dsa.c, line 611, just before the failed check :

Breakpoint 9, DSA_VerifyDigest (key=0x800000010001e9f8, signature=0x800003ffff7f1a30, digest=0x800003ffff7f18c0) at dsa.c:611
611         if (mp_cmp(&v, &r_)) {
40: /x *p.dp = 0xec0736ee31c80291
39: /x *q.dp = 0xed30f48edace915f
38: /x *g.dp = 0xa35c45735028fa2a
37: /x *y.dp = 0xe5fe4f57ad55ca91
36: /x *r_.dp = 0x67997f63f0d767a4
35: /x *s_.dp = 0xfaef14674d663572
34: /x *u1.dp = 0x29656f0dcf27d4c7
33: /x *u2.dp = 0xc47b3fe5a23d13f0
32: /x *v.dp = 0x67997f63f0d767a4
31: /x *w.dp = 0xec25528a9cdd552b
(gdb) c
Continuing.
Verification self-test for dsa passed.

Breakpoint 9, 
Breakpoint 10, DSA_VerifyDigest (key=0x8000000100023218, signature=0x800003ffff7f1a30, digest=0x800003ffff7f18c0) at dsa.c:611
611         if (mp_cmp(&v, &r_)) {
40: /x *p.dp = 0x9ac9c7e9d8bde283
39: /x *q.dp = 0x9bfeeaea14156495
38: /x *g.dp = 0xa6442c63c4560b61
37: /x *y.dp = 0x1266123d4db07edb
36: /x *r_.dp = 0x2058448bd8b284c0
35: /x *s_.dp = 0x7c1ad7cc3da83fde
34: /x *u1.dp = 0xe4249c45da6cf09a
33: /x *u2.dp = 0xee243a8a67e49057
32: /x *v.dp = 0x2058448bd8b284c0
31: /x *w.dp = 0x97ac847dde20d91
(gdb) c
Continuing.
Verification self-test for dsa passed.

Breakpoint 9, 
Breakpoint 10, DSA_VerifyDigest (key=0x8000000100025788, signature=0x800003ffff7f1a30, digest=0x800003ffff7f18c0) at dsa.c:611
611         if (mp_cmp(&v, &r_)) {
40: /x *p.dp = 0x90f72d94e7447bff
39: /x *q.dp = 0x8475624abbb26edd
38: /x *g.dp = 0x15b1a3daab6be83f
37: /x *y.dp = 0x29f13ee26dd330ca
36: /x *r_.dp = 0x4fb26065833a4d8e
35: /x *s_.dp = 0x32a24a1f957b3a1b
34: /x *u1.dp = 0xddf3f99bd3ed0997
33: /x *u2.dp = 0x2a91ea3fec08017b
32: /x *v.dp = 0x4fb26065833a4d8e
31: /x *w.dp = 0xdcafec0193924bb7
(gdb) c
Continuing.
Verification self-test for dsa passed.

Breakpoint 9, 
Breakpoint 10, DSA_VerifyDigest (key=0x8000000100029150, signature=0x800003ffff7f1a30, digest=0x800003ffff7f18c0) at dsa.c:611
611         if (mp_cmp(&v, &r_)) {
40: /x *p.dp = 0xb114a7f981b8483
39: /x *q.dp = 0x8b7c6701ad8a5d99
38: /x *g.dp = 0x44aa44245686349e
37: /x *y.dp = 0xc6203183b27b2a47
36: /x *r_.dp = 0xf75f881276cfd26a
35: /x *s_.dp = 0x5c67fcdb64d12453
34: /x *u1.dp = 0x79fbaac6165bfc4e
33: /x *u2.dp = 0xd210bf367e7718c4
32: /x *v.dp = 0xf75f881276cfd26a
31: /x *w.dp = 0xdcdf89e7d2f128a3
(gdb) c
Continuing.
Verification self-test for dsa passed.

Breakpoint 9, 
Breakpoint 10, DSA_VerifyDigest (key=0x800000010002b490, signature=0x800003ffff7f1a30, digest=0x800003ffff7f18c0) at dsa.c:611
611         if (mp_cmp(&v, &r_)) {
40: /x *p.dp = 0x67fdd1609243373f
39: /x *q.dp = 0xafab20820d2c4755
38: /x *g.dp = 0x11b9de86073d0bed
37: /x *y.dp = 0x8dcc0875bd711e7b
36: /x *r_.dp = 0xdb45a5da73ce7bde
35: /x *s_.dp = 0xdcc8f38769737f01
34: /x *u1.dp = 0x230047222d80c95f
33: /x *u2.dp = 0x7c0b5256089bafa7
32: /x *v.dp = 0xdb45a5da73ce7bde
31: /x *w.dp = 0x4ae98c04e21fa6d0
(gdb) c
Continuing.
Verification self-test for dsa passed.

Breakpoint 9, 
Breakpoint 10, DSA_VerifyDigest (key=0x800000010002daf0, signature=0x800003ffff7f1a30, digest=0x800003ffff7f18c0) at dsa.c:611
611         if (mp_cmp(&v, &r_)) {
40: /x *p.dp = 0xc02fb9fa5cefaab5
39: /x *q.dp = 0x6cf4cf67ecc56b7
38: /x *g.dp = 0xac7dbdfd2ec99b4b
37: /x *y.dp = 0xefd517fe20f309da
36: /x *r_.dp = 0xbab5226b079d9953
35: /x *s_.dp = 0x57ae89e162aea616
34: /x *u1.dp = 0xe1556d7706ff8133
33: /x *u2.dp = 0x28b9d75fe1293e08
32: /x *v.dp = 0xbab5226b079d9953
31: /x *w.dp = 0xd4d6508e3485e021
(gdb) c
Continuing.
Verification self-test for dsa passed.
c
Breakpoint 9, 
Breakpoint 10, DSA_VerifyDigest (key=0x8000000100031878, signature=0x800003ffff7f1a30, digest=0x800003ffff7f18c0) at dsa.c:611
611         if (mp_cmp(&v, &r_)) {
40: /x *p.dp = 0x9feadc194e00f8e5
39: /x *q.dp = 0xfcf059e884d31b1
38: /x *g.dp = 0xb9d3e61b617d1896
37:
 /x *y.dp = 0x68eee06e6623f5cb
36: /x *r_.dp = 0x527d4cf6328738c8
35: /x *s_.dp = 0xdcc101dc9f81d31f
34: /x *u1.dp = 0xc502b06cbf73f2c2
33: /x *u2.dp = 0x8f451e93c238234d
32: /x *v.dp = 0x527d4cf6328738c8
31: /x *w.dp = 0x2b3816d4cfa59864
(gdb) c
Continuing.
Verification self-test for dsa passed.

Breakpoint 9, 
Breakpoint 10, DSA_VerifyDigest (key=0x80000001000338a8, signature=0x800003ffff7f1a30, digest=0x800003ffff7f18c0) at dsa.c:611
611         if (mp_cmp(&v, &r_)) {
40: /x *p.dp = 0xdee971ad660729ad
39: /x *q.dp = 0x6037310348c1aa11
38: /x *g.dp = 0xab336acfb9572408
37: /x *y.dp = 0x2be05bef02bdbc52
36: /x *r_.dp = 0x527d4cf6328738c8
35: /x *s_.dp = 0xdcc101dc9f81d31f
34: /x *u1.dp = 0xe8ceff2084da959d
33: /x *u2.dp = 0x36186341fb2f21dc
32: /x *v.dp = 0x2083ea467693e767
31: /x *w.dp = 0x14f2de39bda4aeb
(gdb) c
Continuing.
bltest: ERR -8182 (Peer's certificate has an invalid signature.) at line 2277.


Here is the same thing on Linux x64 for the same test. freebl was rebuilt without any special MPI assembly optimizations by commenting things in the Makefile. This makes the mpi structures the same on both platforms .

Linux gdb log :


Breakpoint 5, DSA_VerifyDigest (key=0x6655e0, signature=0x7fffffffdc50, digest=0x7fffffffdae0) at dsa.c:611
611         if (mp_cmp(&v, &r_)) {
33: /x *p.dp = 0xec0736ee31c80291
32: /x *q.dp = 0xed30f48edace915f
31: /x *g.dp = 0xa35c45735028fa2a
30: /x *y.dp = 0xe5fe4f57ad55ca91
29: /x *r_.dp = 0x67997f63f0d767a4
28: /x *s_.dp = 0xfaef14674d663572
27: /x *u1.dp = 0x29656f0dcf27d4c7
26: /x *u2.dp = 0xc47b3fe5a23d13f0
25: /x *v.dp = 0x67997f63f0d767a4
24: /x *w.dp = 0xec25528a9cdd552b
(gdb) c
Continuing.
Verification self-test for dsa passed.

Breakpoint 5, DSA_VerifyDigest (key=0x667b60, signature=0x7fffffffdc50, digest=0x7fffffffdae0) at dsa.c:611
611         if (mp_cmp(&v, &r_)) {
33: /x *p.dp = 0x9ac9c7e9d8bde283
32: /x *q.dp = 0x9bfeeaea14156495
31: /x *g.dp = 0xa6442c63c4560b61
30: /x *y.dp = 0x1266123d4db07edb
29: /x *r_.dp = 0x2058448bd8b284c0
28: /x *s_.dp = 0x7c1ad7cc3da83fde
27: /x *u1.dp = 0xe4249c45da6cf09a
26: /x *u2.dp = 0xee243a8a67e49057
25: /x *v.dp = 0x2058448bd8b284c0
24: /x *w.dp = 0x97ac847dde20d91
(gdb) c
Continuing.
Verification self-test for dsa passed.

Breakpoint 5, DSA_VerifyDigest (key=0x66a0c0, signature=0x7fffffffdc50, digest=0x7fffffffdae0) at dsa.c:611
611         if (mp_cmp(&v, &r_)) {
33: /x *p.dp = 0x90f72d94e7447bff
32: /x *q.dp = 0x8475624abbb26edd
31: /x *g.dp = 0x15b1a3daab6be83f
30: /x *y.dp = 0x29f13ee26dd330ca
29: /x *r_.dp = 0x4fb26065833a4d8e
28: /x *s_.dp = 0x32a24a1f957b3a1b
27: /x *u1.dp = 0xddf3f99bd3ed0997
26: /x *u2.dp = 0x2a91ea3fec08017b
25: /x *v.dp = 0x4fb26065833a4d8e
24: /x *w.dp = 0xdcafec0193924bb7
(gdb) c
Continuing.
Verification self-test for dsa passed.

Breakpoint 5, DSA_VerifyDigest (key=0x66d970, signature=0x7fffffffdc50, digest=0x7fffffffdae0) at dsa.c:611
611         if (mp_cmp(&v, &r_)) {
33: /x *p.dp = 0xb114a7f981b8483
32: /x *q.dp = 0x8b7c6701ad8a5d99
31: /x *g.dp = 0x44aa44245686349e
30: /x *y.dp = 0xc6203183b27b2a47
29: /x *r_.dp = 0xf75f881276cfd26a
28: /x *s_.dp = 0x5c67fcdb64d12453
27: /x *u1.dp = 0x79fbaac6165bfc4e
26: /x *u2.dp = 0xd210bf367e7718c4
25: /x *v.dp = 0xf75f881276cfd26a
24: /x *w.dp = 0xdcdf89e7d2f128a3
(gdb) c
Continuing.
Verification self-test for dsa passed.

Breakpoint 5, DSA_VerifyDigest (key=0x66fb90, signature=0x7fffffffdc50, digest=0x7fffffffdae0) at dsa.c:611
611         if (mp_cmp(&v, &r_)) {
33: /x *p.dp = 0x67fdd1609243373f
32: /x *q.dp = 0xafab20820d2c4755
31: /x *g.dp = 0x11b9de86073d0bed
30: /x *y.dp = 0x8dcc0875bd711e7b
29: /x *r_.dp = 0xdb45a5da73ce7bde
28: /x *s_.dp = 0xdcc8f38769737f01
27: /x *u1.dp = 0x230047222d80c95f
26: /x *u2.dp = 0x7c0b5256089bafa7
25: /x *v.dp = 0xdb45a5da73ce7bde
24: /x *w.dp = 0x4ae98c04e21fa6d0
(gdb) c
Continuing.
Verification self-test for dsa passed.

Breakpoint 5, DSA_VerifyDigest (key=0x6720f0, signature=0x7fffffffdc50, digest=0x7fffffffdae0) at dsa.c:611
611         if (mp_cmp(&v, &r_)) {
33: /x *p.dp = 0xc02fb9fa5cefaab5
32: /x *q.dp = 0x6cf4cf67ecc56b7
31: /x *g.dp = 0xac7dbdfd2ec99b4b
30: /x *y.dp = 0xefd517fe20f309da
29: /x *r_.dp = 0xbab5226b079d9953
28: /x *s_.dp = 0x57ae89e162aea616
27: /x *u1.dp = 0xe1556d7706ff8133
26: /x *u2.dp = 0x28b9d75fe1293e08
25: /x *v.dp = 0xbab5226b079d9953
24: /x *w.dp = 0xd4d6508e3485e021
(gdb) c
Continuing.
Verification self-test for dsa passed.

Breakpoint 5, DSA_VerifyDigest (key=0x675d60, signature=0x7fffffffdc50, digest=0x7fffffffdae0) at dsa.c:611
611         if (mp_cmp(&v, &r_)) {
33: /x *p.dp = 0x9feadc194e00f8e5
32: /x *q.dp = 0xfcf059e884d31b1
31: /x *g.dp = 0xb9d3e61b617d1896
30: /x *y.dp = 0x68eee06e6623f5cb
29: /x *r_.dp = 0x527d4cf6328738c8
28: /x *s_.dp = 0xdcc101dc9f81d31f
27: /x *u1.dp = 0xc502b06cbf73f2c2
26: /x *u2.dp = 0x8f451e93c238234d
25: /x *v.dp = 0x527d4cf6328738c8
24: /x *w.dp = 0x2b3816d4cfa59864
(gdb) c
Continuing.
Verification self-test for dsa passed.

Breakpoint 5, DSA_VerifyDigest (key=0x677d80, signature=0x7fffffffdc50, digest=0x7fffffffdae0) at dsa.c:611
611         if (mp_cmp(&v, &r_)) {
33: /x *p.dp = 0xdee971ad660729ad
32: /x *q.dp = 0x6037310348c1aa11
31: /x *g.dp = 0x87b740b8f563d8cb
30: /x *y.dp = 0x61a32e6215a0ea79
29: /x *r_.dp = 0x7c4a948b1f4da6b9
28: /x *s_.dp = 0xab0dd543d4125bd1
27: /x *u1.dp = 0xae8d5953e60b2537
26: /x *u2.dp = 0xd4ed3308b342ab12
25: /x *v.dp = 0x7c4a948b1f4da6b9
24: /x *w.dp = 0x1972163cdb443fd0
(gdb) 

As you can see, for the last iteration, the following are different between HP-UX and Linux :
g
y
r_
s_
u1
u2
v
w

Only the p and q are the same !

And interestingly, the value 0x527d4cf6328738c8 appears as the r_ in the last two iterations on HP-UX .

Comment 5

[Julien Pierre]

Continuing to debug this thorny issue where I picked up last week.
I tried to figure out why the g was different on HP-UX .

One observation is that the bignums that have wrong values on HP-UX are not deterministic. This suggests some uninitialized memory is in play.

I dumped the memory content of the SECItems in key for prime, subPrime and base.
They were all identical between Linux and HP-UX.

Something is going wrong on this line in dsa.c :

564     SECITEM_TO_MPINT(key->params.base,     &g);

Somehow, the result g bignum is different on HP-UX from its expected value.

Comment 6

[Julien Pierre]

Turns out the "g" bignum gets reused during the computations, so that is not the line where the problem occurs.

Rather, it is the r_ and s_ from the signature which differ .

Those are fetched here :
67     ** Convert received signature (r', s') into MPI integers.
568     */
569     OCTETS_TO_MPINT(signature->data, &r_, dsa_subprime_len);
570     OCTETS_TO_MPINT(signature->data + dsa_subprime_len, &s_, dsa_subprime_len);

The signature is passed in. I have dumped the memory and observed that it is different between Linux and HP-UX . The key and digest that are input are both the same.

This suggests it might be a problem with the test rather than the mpi code. I'm still trying to figure out where exactly the signature comes from in this test. This is in test ciphertext7 .

Comment 7

[Julien Pierre]

It is indeed a problem in the test data.

The problem turns out to be an issue with file descriptors. On HP-UX, the default limit for FDs is 60.

When blapitest gets to ciphertext7, the limit is exceeded.
PR_Open fails in load_file_data .

Unfortunately, this is a void function, so this error is ignored.

The DSA parameter structure thus remains the same, and the next test runs, but continues to use the old structures.

The "right" fix is to change load_file_data to return something non-void, and have all its callers check that status. Since many of them also return void, they would have to be changed as well.

The simpler fix would be to just check for I/O errors and abort the program when they occur.

I will attach a patch that checks for this PR_Open problem.

Comment 8

[Julien Pierre]

Attachment: Check that PR_Open succeeds

This patch produces the following output for the test case in question when the FD table is full :

Verification self-test for dsa passed.
Verification self-test for dsa passed.
Verification self-test for dsa passed.
Verification self-test for dsa passed.
Verification self-test for dsa passed.
Verification self-test for dsa passed.
Verification self-test for dsa passed.
Verification self-test for dsa passed.
bltest: ERR -5971 (Process open FD table is full) at line 2880.

Comment 9

[Robert Relyea]

Comment on attachment 809790 [details] [diff] [review]
Check that PR_Open succeeds

r+ rrelyea.

So the problem was we added more tests. The next question is, why aren't we closing the file handles at the end of the test?

bob

Comment 10

[Wan-Teh Chang]

Comment on attachment 809790 [details] [diff] [review]
Check that PR_Open succeeds

Review of attachment 809790 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for tracking this down and writing a patch.

::: blapitest.c
@@ +2877,5 @@
>      data->pBuf.data = NULL;
>      data->pBuf.len = 0;
>      file = PR_Open(fn, PR_RDONLY, 00660);
> +    CHECKERROR(!file, __LINE__);
> +    setupIO(arena, data, file, NULL, 0);

1. Please add
    PR_Close(file);

2. Please write the CHECKERROR as follows:

    CHECKERROR(file ? SECSuccess : SECFailure, __LINE__);

because the CHECKERROR expects its first argument to be a
SECStatus "rv".

Comment 11

[Julien Pierre]

Fair enough that we should close the FD after use, I should have included it in the patch, but debugging at 4am has a way of making you forget things.

Wan-Teh,

Re: CHECKERROR is a macro, not a function :

45 #define CHECKERROR(rv, ln) \
46     if (rv) { \
47 	PRErrorCode prerror = PR_GetError(); \
48 	PR_fprintf(PR_STDERR, "%s: ERR %d (%s) at line %d.\n", progName, \
49 	prerror, PORT_ErrorToString(prerror), ln); \
50 	exit(-1); \
51     }

The macro is not type-dependent. Values of 0 (SECSuccess) mean "no error", and all other values mean "error" . I believe the way I wrote it works fine. Your way works too, but does not change the meaning.
If we want to enforce the type, perhaps CHECKERROR should be changed to a function.

Comment 12

[Julien Pierre]

Attachment: Updated patch

Here is a new patch. Modifications :
1) I changed CHECKERROR to a function. This reduces the binary size.
2) CHECKERROR is written with a SECStatus in mind, as suggested . I added one for every PR_Open that wasn't previously following by error checking code
3) I removed the PR_Close from setupIO in the error code
4) I added a PR_Close in load_file_data

This patch solves the current problem and should also help with all other file open problems in the future.

I have inspected the code and I don't believe there are any remaining cases of FD leaks.

Comment 13

[Ryan Sleevi]

Comment on attachment 810353 [details] [diff] [review]
Updated patch

Review of attachment 810353 [details] [diff] [review]:
-----------------------------------------------------------------

::: blapitest.c
@@ +2878,4 @@
>      data->pBuf.data = NULL;
>      data->pBuf.len = 0;
>      file = PR_Open(fn, PR_RDONLY, 00660);
> +    CHECKERROR(file ? SECSuccess : SECFailure, __LINE__);

Note: The existing tests gracefully handle if the file is not found, by zeroizing the buffer. In my patch for https://bugzilla.mozilla.org/show_bug.cgi?id=836019 , I explicitly take advantage of this.

An alternative would be to return bool in load_file_data, and to check it when you actually do want to error out (for example, on command-line option flags)

Comment 14

[Julien Pierre]

Ryan,

As I mentioned in comment 7, 
"The "right" fix is to change load_file_data to return something non-void, and have all its callers check that status. Since many of them also return void, they would have to be changed as well."

I didn't do that because I felt my change was much simpler.
It seems that you are now adding tests that have some optional files. That was not the case when I wrote my patch. In that case, an alternate fix would need to be done.

Perhaps you could pass an extra "PRBool optional" argument to load_file_data in order for this check to be skipped.

Comment 15

[Wan-Teh Chang]

Comment on attachment 810353 [details] [diff] [review]
Updated patch

Review of attachment 810353 [details] [diff] [review]:
-----------------------------------------------------------------

::: blapitest.c
@@ +2878,4 @@
>      data->pBuf.data = NULL;
>      data->pBuf.len = 0;
>      file = PR_Open(fn, PR_RDONLY, 00660);
> +    CHECKERROR(file ? SECSuccess : SECFailure, __LINE__);

We can allow PR_FILE_NOT_FOUND_ERROR to support optional test input files.

Comment 16

[Wan-Teh Chang]

Attachment: Patch v3, by Julien Pierre

Julien, Ryan, please review this patch. It should meet both of your
requirements.

I made the following changes to Julien's patch.

1. I left the CHECKERROR macro unchanged. I added a new macro CHECKFILE
that checks if a PRFileDesc * pointer is null.

2. CHECKFILE allows a null |file| argument if the error is
PR_FILE_NOT_FOUND_ERROR. This should allow Ryan to use optional test
input files. I am not familiar enough with blapitest.c to tell if
this is the best way to handle optional test input files.

Comment 17

[Wan-Teh Chang]

Attachment: Patch v3.1, by Julien Pierre

I added a comment to explain why CHECKFILE allows PR_FILE_NOT_FOUND_ERROR.

Comment 18

[Julien Pierre]

Comment on attachment 822492 [details] [diff] [review]
Patch v3.1, by Julien Pierre

I have some concerns about this approach.

If the test files are missing for some reason, but they are actually required for the tests in question, the problematic behavior that I observed when running out of file descriptors, where the data for the last file is reused for the next test, will continue to occur. 

Is there a way for the caller to know ahead of time if test files are required or optional ?
If so, I think it would be preferable to add the "PRBool optional" argument  to load_file_data, and write CHECKFILE the following way :

+/* "file" must be a PRFileDesc * pointer. PR_FILE_NOT_FOUND_ERROR is allowed
+ * because it may indicate an optional test input file is absent. */
+#define CHECKFILE(file, ln, optional) \
+    if (!(file)) { \
+	PRErrorCode prerror = PR_GetError(); \
+	if (!optional || prerror != PR_FILE_NOT_FOUND_ERROR) { \
+	    PR_fprintf(PR_STDERR, "%s: ERR %d (%s) at line %d.\n", progName, \
+		       prerror, PORT_ErrorToString(prerror), ln); \
+	    exit(-1); \
+	} \
+    }
+

Comment 19

[Ryan Sleevi]

Comment on attachment 822492 [details] [diff] [review]
Patch v3.1, by Julien Pierre

Review of attachment 822492 [details] [diff] [review]:
-----------------------------------------------------------------

::: cmd/bltest/blapitest.c
@@ +2888,5 @@
>      data->pBuf.data = NULL;
>      data->pBuf.len = 0;
>      file = PR_Open(fn, PR_RDONLY, 00660);
> +    CHECKFILE(file, __LINE__);
> +    setupIO(arena, data, file, NULL, 0);

This will always result in a SECFailure being returned (and ignored) when !file. Is that intended? Seems fine to do, just wanted to confirm.

Comment 20

[Wan-Teh Chang]

Comment on attachment 822492 [details] [diff] [review]
Patch v3.1, by Julien Pierre

Review of attachment 822492 [details] [diff] [review]:
-----------------------------------------------------------------

::: cmd/bltest/blapitest.c
@@ +2888,5 @@
>      data->pBuf.data = NULL;
>      data->pBuf.len = 0;
>      file = PR_Open(fn, PR_RDONLY, 00660);
> +    CHECKFILE(file, __LINE__);
> +    setupIO(arena, data, file, NULL, 0);

If |file| is null, CHECKFILE will call exit() except when
the error code is PR_FILE_NOT_FOUND_ERROR. The exception
for PR_FILE_NOT_FOUND_ERROR is intended to allow a missing
optional test input file.

I can remove the exception for PR_FILE_NOT_FOUND_ERROR, so
we can discuss what's the best way to handle optional test
input files. For your RSA-OAEP and RSA-PSS tests, can you
use empty input files rather than allowing certain input
files to be missing?

Comment 21

[Julien Pierre]

Oh, and another comment is that there is no good reason for CHECKFILE (or CHECKERROR) to be a macro.
Doing so just increases the code size and makes things harder to step through in a debugger.

Comment 22

[Wan-Teh Chang]

Julien: you are right. I should have explained why I kept CHECKERROR
and CHECKFILE as macros -- If we want to change them to functions, we
also need to rename them CheckError and CheckFile to conform to our
naming convention. I was too lazy to do that.

Comment 23

[Wan-Teh Chang]

Attachment: Patch v4, by Julien Pierre: make an exception for PR_FILE_NOT_FOUND_ERROR

Ryan: I finally get your question in comment 19. You are right.
To preserve the behavior of allowing test input files to be missing,
we should only call setupIO (and PR_Close) only if |file| is not null.
I discovered this bug when I tested the patch. I fixed the bug in
this patch.

I also found that cmd/bltest/tests/camellia_ecb/ and
cmd/bltest/tests/camellia_cbc/ have missing test input files.
Both are missing plaintext1 and plaintext2, and camellia_cbc is also
missing iv1 and iv2. However, I am surprised at what the missing files
mean. A missing input file is not the same as an empty input file!

1. If a "plaintext" input file is missing, we will continue to use
the last present "plaintext" file. So, a missing "plaintext1" or
"plaintext2" means the contents of "plaintext0" will get used. This
is because of the bltestCopyIO(arena, &cipherInfo.input, &pt) call:

void
bltestCopyIO(PLArenaPool *arena, bltestIO *dest, bltestIO *src)
{
    SECITEM_CopyItem(arena, &dest->buf, &src->buf);
    if (src->pBuf.len > 0) {
        dest->pBuf.len = src->pBuf.len;
        dest->pBuf.data = dest->buf.data + (src->pBuf.data - src->buf.data);
    }
    dest->mode = src->mode;
    dest->file = src->file;
}

If a "plaintext" input file is missing, src->pBuf.len will be 0
(set by load_file_data), so dest->pBuf is not modified. Note that
&pt is src and &cipherInfo.input is dest.

2. If an "iv" input file is missing, we will continue to use
the last present "iv" file. This is because load_file_data
doesn't call setupIO if |file| is null, so params->sk.iv.buf
is not modified. (bltest_camellia_init gets the IV from
cipherInfo->params.sk.iv.buf

Ryan, if your RSA-OAEP and RSA-PSS tests do not interpret
missing test input files in the same way, then I think we
should avoid optional test input files.

Comment 24

[Wan-Teh Chang]

Attachment: Alt patch, by Julien Pierre: add missing Camellia test input files, disallow any PR_Open failure

I suggest that we first add the missing Camellia test input files
and check in this patch that disallows any PR_Open failure.

If this patch is checked in, I will create a patch that adds an
exception for PR_FILE_NOT_FOUND_ERROR so that Ryan can have optional
test input files (if he needs them).

Comment 25

[Wan-Teh Chang]

Attachment: output.log excerpt that demonstrates blapitest reuses the previous "plaintext" or "iv" file contents if a "plaintext" or "iv" input file is missing

I added some debugging printf statements to blapitest.c
and ran all.sh. This excerpt of the output.log file
demonstrates what I described in comment 23: if "plaintext1"
is missing, the contents of "plaintext0" get used, and if
"iv1" is missing, the contents of "iv0" get used.

The code in blapitest.c that allows this to happen was
added in year 2000, well before we added the Camellia
tests:

  1.20 <[e-mail]> 2000-11-30 17:24
  massive changes to blapitest to allow for more flexible
  input types and improved performance testing output.

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/security/nss/cmd/bltest&command=DIFF_FRAMESET&file=blapitest.c&rev2=1.20&rev1=1.19

It is not clear whether this behavior is intentional.

I also verified that the plaintext1 and iv1 files are missing
in the original patch in bug 361025.

Comment 26

[Wan-Teh Chang]

Attachment: File descriptor leak fix, by Julien Pierre

I checked in Julien's file descriptor leak fix first:
https://hg.mozilla.org/projects/nss/rev/e47fcade843b

Comment 27

[Ryan Sleevi]

Comment on attachment 825008 [details] [diff] [review]
Alt patch, by Julien Pierre: add missing Camellia test input files, disallow any PR_Open failure

Review of attachment 825008 [details] [diff] [review]:
-----------------------------------------------------------------

r=ryan.sleevi

I am fine with making it a critical failure. I'll update my patch accordingly to handle 'empty' files, rather than using missing files to be equivalent.

Comment 28

[Ryan Sleevi]

Comment on attachment 825001 [details] [diff] [review]
Patch v4, by Julien Pierre: make an exception for PR_FILE_NOT_FOUND_ERROR

Review of attachment 825001 [details] [diff] [review]:
-----------------------------------------------------------------

Let's skip the exception approach; I'll update my patch.

Comment 29

[Kai Engert (:KaiE:)]

changing target milestone to 3.15.4

Comment 30

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last months and this bug has priority 'P2'.
:beurdouche, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 31

[Marco Castelluccio [:marco]]

We have modified the bot to only consider P1 as high priority, so I'm cancelling the needinfo here.

Comment 32

[test]

Attachment: test3.svg

Comment 33

[:glob ✱]

The content of attachment 9332294 [details] has been deleted for the following reason:

Abuse