Last Comment Bug 485127 - bltest crashes when attempting rc5_cbc or rc5_ecb
: bltest crashes when attempting rc5_cbc or rc5_ecb
Product: NSS
Classification: Components
Component: Tools (show other bugs)
: trunk
: All All
: P2 normal (vote)
: 3.12.3
Assigned To: Julien Pierre
Depends on:
  Show dependency treegraph
Reported: 2009-03-24 22:18 PDT by Julien Pierre
Modified: 2009-03-25 18:44 PDT (History)
0 users
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

Fix crash (checked in) (2.50 KB, patch)
2009-03-24 22:46 PDT, Julien Pierre
nelson: review+
Details | Diff | Splinter Review

Description Julien Pierre 2009-03-24 22:18:51 PDT
I was trying to do some quick performance testing of all algorithms. I kept getting core files with RC5.

I discovered that RC5 is not in softoken and seems not to have been there for a while. The code is surrounded with #ifdef NSS_SOFTOKEN_DOES_RC5 .

But bltest still accepts commands like :

./bltest -E -m rc5_cbc -g 16 -p 100 -4 2 -i ./test -o /dev/null
./bltest -E -m rc5_ecb -g 16 -p 100 -4 2 -i ./test -o /dev/null

And they will crash with an obscure stack :

  [1] 0x0(0x0, 0x0, 0xfffffd7ffebcef08, 0x0, 0x7379a0, 0x20000), at 0x0
  [2] cipherDoOp(cipherInfo = 0x716540), line 2338 in "blapitest.c"
  [3] ThreadExecTest(data = 0x716540), line 3129 in "blapitest.c"
=>[4] _pt_root(arg = 0x717970), line 228 in "ptthread.c"
  [5] _thr_setup(0x0, 0x0, 0x0, 0x0, 0x0, 0x0), at 0xfffffd7fff006deb
  [6] _lwp_start(0x0, 0x0, 0x0, 0x0, 0x0, 0x0), at 0xfffffd7fff007020

The crash is because the content of the cipherInfo is actually invalid.

The fix should be to refuse to do those mechanisms that are not implemented and give a usage page.

Incidently, the usage page should probably mention the list of the supported mechanisms. That would make things much easier. bltest -H has a list of ECC curves, but not mechanisms.
Comment 1 Julien Pierre 2009-03-24 22:46:07 PDT
Created attachment 369221 [details] [diff] [review]
Fix crash (checked in)
Comment 2 Nelson Bolyard (seldom reads bugmail) 2009-03-25 13:33:33 PDT
Comment on attachment 369221 [details] [diff] [review]
Fix crash (checked in)

Two issues:


These tests should all be consistent, either #if or #ifdef.

>@@ -2918,7 +2925,6 @@ blapi_selftest(bltestCipherMode *modes, 
>     finished = PR_FALSE;
>     nummodes = (numModes == 0) ? NUMMODES : numModes;
>     for (i=0; i < nummodes && !finished; i++) {
>-	if (i == bltestRC5_ECB || i == bltestRC5_CBC) continue;

That should be ifdef'ed, not deleted, I believe.
Comment 3 Julien Pierre 2009-03-25 18:13:07 PDT

1. That's a cosmetic issue, I can fix it at checkin

2. No, actually it is fine to delete it. The RC5 options themselves are ifdef'ed above in the source file. Therefore, by deleting this statement, if NSS_SOFTOKEN_DOES_RC5 is not defined, there is no need to skip any test, since the RC5 option won't be encountered. And if it is defined, it will be encountered, and with the statement deletion, it will now be run where it wasn't before, which I think is correct. So, please reconsider your review on this patch.
Comment 4 Nelson Bolyard (seldom reads bugmail) 2009-03-25 18:23:04 PDT
Comment on attachment 369221 [details] [diff] [review]
Fix crash (checked in)

I agree that the first of the two issues is something that, by itself, didn't require r-.  I gave r- for the second.  But you've convinced me that the second is OK, so r+, provided you fix the if/ifdef issue.
Comment 5 Julien Pierre 2009-03-25 18:41:46 PDT
Comment on attachment 369221 [details] [diff] [review]
Fix crash (checked in)

Thanks, Nelson.

I checked in the patch with the #if changed to #ifdef.

Checking in blapitest.c;
/cvsroot/mozilla/security/nss/cmd/bltest/blapitest.c,v  <--  blapitest.c
new revision: 1.57; previous revision: 1.56

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