1404421 - Add an empty slot to the test PKCS#11 module

Status: RESOLVED FIXED

(Core :: Security: PSM, enhancement, P3)

Description

[Wouter Verhelst :wouter]

As suggested in #1357391 comment 134, it would make sense to add a third slot to the test PKCS#11 module, so that the case of 'a slot without token exists' can be reliably tested.

The current module has two slots; one always has a token, the other has one until a short time after the next C_WaitForSlotEvent() call. This makes it an unreliable option to test the 'no token found' case, which means the new browser.pkcs11 API introduced with #1357391 is not tested for that case.

A third slot should be added, one that does not ever have a token, so that three testing for that API (and possibly others as well) can be improved.

Comment 1

[Wouter Verhelst :wouter]

Attachment: Bug 1404421 - Add an empty slot to the test PKCS#11 module

It is helpful to have a slot which never has a token, so that the
absense of a token can be asserted in unit tests.

Add a third token that is always empty, add an assertion for that to the
test of the browser.pkcs11 API, and update the pkcs11 module test in PSM
so that it doesn't fail with the new slot present.

Review commit: https://reviewboard.mozilla.org/r/190208/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/190208/

Comment 2

[Wouter Verhelst :wouter]

anyone care to review that?

Comment 3

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8919349 [details]
Bug 1404421 - Add an empty slot to the test PKCS#11 module

https://reviewboard.mozilla.org/r/190208/#review195628

Nice! This looks good, although I have some suggestions for improvements. In particular, it would be nice to have a case that ensures we get the status SLOT_NOT_PRESENT for that slot (see https://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/tests/unit/test_pkcs11_slot.js - I would basically copy/paste the lines specific to slot 2 and update them for the empty slot you're adding).

::: security/manager/ssl/tests/unit/pkcs11testmodule/pkcs11testmodule.cpp:84
(Diff revision 1)
>      return CKR_ARGUMENTS_BAD;
>    }
>  
> -  // Slot 2 is always present, while slot 1 may or may not be present.
> +  // Slot 3 is never present, slot 2 is always present, while slot 1 may or may
> +  // not be present.
>    CK_ULONG slotCount = (!limitToTokensPresent || tokenPresent ? 1 : 0) + 1;

I think the ternary operator and the additional if statement make this a bit confusing now. How about we just break it out into 3 cases:
* slotCount is always at least 1 because of slot 2
* we increase it by 1 if slot 1 is present (or if we want non-present tokens as well)
* we again increase it by 1 if we want non-present tokens as well

::: security/manager/ssl/tests/unit/pkcs11testmodule/pkcs11testmodule.cpp:85
(Diff revision 1)
>    }
>  
> -  // Slot 2 is always present, while slot 1 may or may not be present.
> +  // Slot 3 is never present, slot 2 is always present, while slot 1 may or may
> +  // not be present.
>    CK_ULONG slotCount = (!limitToTokensPresent || tokenPresent ? 1 : 0) + 1;
> +  if (!limitToTokensPresent) slotCount++;

style nit - always use braces:
```
if (!limitToTokensPresent) {
  slotCount++;
}
```

::: security/manager/ssl/tests/unit/pkcs11testmodule/pkcs11testmodule.cpp:112
(Diff revision 1)
> -      pSlotList[0] = 1;
> +        pSlotList[0] = 1;
> -      pSlotList[1] = 2;
> +        pSlotList[1] = 2;
> +        pSlotList[2] = 3;
> +        break;
> +      default:
> +        // how did we get here?!?

`MOZ_ASSERT_UNREACHABLE("unexpected slot count in Test_C_GetSlotList");`

Comment 4

[Wouter Verhelst :wouter]

Comment on attachment 8919349 [details]
Bug 1404421 - Add an empty slot to the test PKCS#11 module

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/190208/diff/1-2/

Comment 5

[Wouter Verhelst :wouter]

Comment on attachment 8919349 [details]
Bug 1404421 - Add an empty slot to the test PKCS#11 module

https://reviewboard.mozilla.org/r/190208/#review195628

> I think the ternary operator and the additional if statement make this a bit confusing now. How about we just break it out into 3 cases:
> * slotCount is always at least 1 because of slot 2
> * we increase it by 1 if slot 1 is present (or if we want non-present tokens as well)
> * we again increase it by 1 if we want non-present tokens as well

That's essentially what we're doing with the ternary, but I agree that it's confusing

I broke it out differently. I think that's easier to follow.

Comment 6

[Wouter Verhelst :wouter]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #3)
> In
> particular, it would be nice to have a case that ensures we get the status
> SLOT_NOT_PRESENT for that slot (see
> https://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/tests/
> unit/test_pkcs11_slot.js - I would basically copy/paste the lines specific
> to slot 2 and update them for the empty slot you're adding).

Er, you meant the flag, not just the absense of the token. I guess it was a bit too early in the morning ;-)

Let me try that again.

Comment 7

[Wouter Verhelst :wouter]

Comment on attachment 8919349 [details]
Bug 1404421 - Add an empty slot to the test PKCS#11 module

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/190208/diff/2-3/

Comment 8

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8919349 [details]
Bug 1404421 - Add an empty slot to the test PKCS#11 module

https://reviewboard.mozilla.org/r/190208/#review195964

Great - thanks! Unfortunately I told you to use the wrong assert macro, but that should be a quick fix.

::: security/manager/ssl/tests/unit/pkcs11testmodule/pkcs11testmodule.cpp:25
(Diff revisions 1 - 3)
>  #  include <unistd.h> // for usleep
>  #endif
>  
>  #include "pkcs11.h"
>  
> +#include "mozilla/Assertions.h"

Oh, shoot - sorry. I forgot we've avoided explicitly depending on other Mozilla things in this file. Let's just use <cassert> / assert("...") (put the include just above the one for string)

::: security/manager/ssl/tests/unit/pkcs11testmodule/pkcs11testmodule.cpp:86
(Diff revisions 1 - 3)
>      return CKR_ARGUMENTS_BAD;
>    }
>  
> -  // Slot 3 is never present, slot 2 is always present, while slot 1 may or may
> -  // not be present.
> -  CK_ULONG slotCount = (!limitToTokensPresent || tokenPresent ? 1 : 0) + 1;
> +  // We always return slot 2
> +  CK_ULONG slotCount = 1;
> +  // If we empty slots, we also return slots 1 and 3

nit: "If we want"?

::: security/manager/ssl/tests/unit/pkcs11testmodule/pkcs11testmodule.cpp:92
(Diff revisions 1 - 3)
> -  if (!limitToTokensPresent) slotCount++;
> +  if (!limitToTokensPresent) {
> +    slotCount += 2;
> +  } else {
> +    // If we don't want empty slots, but token 1 is present, return that (but
> +    // not slot 3)
> +    if (tokenPresent) {

style nit: since the else branch only contains the if, we can do an 'else if' here.

::: security/manager/ssl/tests/unit/pkcs11testmodule/pkcs11testmodule.cpp:122
(Diff revisions 1 - 3)
>          pSlotList[0] = 1;
>          pSlotList[1] = 2;
>          pSlotList[2] = 3;
>          break;
>        default:
> -        // how did we get here?!?
> +        MOZ_ASSERT_UNREACHABLE("Unexpected slot count in Test_C_GetSlotList");

So this would just be 'assert("Unexpected slot...");'

Comment 9

[Wouter Verhelst :wouter]

Comment on attachment 8919349 [details]
Bug 1404421 - Add an empty slot to the test PKCS#11 module

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/190208/diff/3-4/

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Sorry, this had to be backed out for eslint failure at security/manager/ssl/tests/unit/test_pkcs11_slot.js:44 and static bustage at security/manager/ssl/tests/unit/pkcs11testmodule/pkcs11testmodule.cpp:119:

https://hg.mozilla.org/integration/autoland/rev/b721cd90de90f188152728bc4005aa892fb659c9

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=9ad123fb9ce5fd3211c6f675e8acbe452296880b&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable

Failure eslint log: https://treeherder.mozilla.org/logviewer.html#?job_id=138001426&repo=autoland
>  TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/security/manager/ssl/tests/unit/test_pkcs11_slot.js:44:1 | Block must not be padded by blank lines. (padded-blocks)

Failure log static: https://treeherder.mozilla.org/logviewer.html#?job_id=138001433&repo=autoland

[task 2017-10-18T22:44:28.503Z] 22:44:28     INFO -  In file included from /builds/worker/workspace/build/src/obj-firefox/security/manager/ssl/tests/unit/pkcs11testmodule/Unified_cpp_pkcs11testmodule0.cpp:2:
[task 2017-10-18T22:44:28.503Z] 22:44:28     INFO -  /builds/worker/workspace/build/src/security/manager/ssl/tests/unit/pkcs11testmodule/pkcs11testmodule.cpp:119:16: error: implicit conversion turns string literal into bool: 'const char [44]' to 'bool' [-Werror,-Wstring-conversion]
[task 2017-10-18T22:44:28.503Z] 22:44:28     INFO -          assert("Unexpected slot count in Test_C_GetSlotList");
[task 2017-10-18T22:44:28.503Z] 22:44:28     INFO -          ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[task 2017-10-18T22:44:28.504Z] 22:44:28     INFO -  /usr/include/assert.h:89:5: note: expanded from macro 'assert'
[task 2017-10-18T22:44:28.504Z] 22:44:28     INFO -    ((expr)                                                               \
[task 2017-10-18T22:44:28.504Z] 22:44:28     INFO -      ^~~~
[task 2017-10-18T22:44:28.504Z] 22:44:28     INFO -  1 error generated.

Comment 11

[Wouter Verhelst :wouter]

Comment on attachment 8919349 [details]
Bug 1404421 - Add an empty slot to the test PKCS#11 module

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/190208/diff/4-5/

Comment 12

[Wouter Verhelst :wouter]

(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #10)
> Sorry, this had to be backed out for eslint failure at
> security/manager/ssl/tests/unit/test_pkcs11_slot.js:44 and static bustage at
> security/manager/ssl/tests/unit/pkcs11testmodule/pkcs11testmodule.cpp:119:
> 
> https://hg.mozilla.org/integration/autoland/rev/
> b721cd90de90f188152728bc4005aa892fb659c9
> 
> Push with failures:
> https://treeherder.mozilla.org/#/
> jobs?repo=autoland&revision=9ad123fb9ce5fd3211c6f675e8acbe452296880b&filter-
> resultStatus=testfailed&filter-resultStatus=busted&filter-
> resultStatus=exception&filter-resultStatus=retry&filter-
> resultStatus=usercancel&filter-resultStatus=runnable
> 
> Failure eslint log:
> https://treeherder.mozilla.org/logviewer.html#?job_id=138001426&repo=autoland
> >  TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/security/manager/ssl/tests/unit/test_pkcs11_slot.js:44:1 | Block must not be padded by blank lines. (padded-blocks)

Whoops, forgot about running eslint. That was sloppy of me, sorry.

> Failure log static:
> https://treeherder.mozilla.org/logviewer.html#?job_id=138001433&repo=autoland
> 
> [task 2017-10-18T22:44:28.503Z] 22:44:28     INFO -  In file included from
> /builds/worker/workspace/build/src/obj-firefox/security/manager/ssl/tests/
> unit/pkcs11testmodule/Unified_cpp_pkcs11testmodule0.cpp:2:
> [task 2017-10-18T22:44:28.503Z] 22:44:28     INFO - 
> /builds/worker/workspace/build/src/security/manager/ssl/tests/unit/
> pkcs11testmodule/pkcs11testmodule.cpp:119:16: error: implicit conversion
> turns string literal into bool: 'const char [44]' to 'bool'
> [-Werror,-Wstring-conversion]
> [task 2017-10-18T22:44:28.503Z] 22:44:28     INFO -         
> assert("Unexpected slot count in Test_C_GetSlotList");

Hrm. I think that's a bit overzealous; this is an "assert that this code is not reached" statement. Yes, this is a C++ source file, which has strong typing; but assert() is a macro, which does not.

Anyway, I've added "== NULL" at the end, which will fix that, will retain the message, and should work (i.e., fail when reached) as expected.

Started a try build to be sure this time around[1], but that revealed some errors on Windows. Don't have time to fix those now, but will do that later.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=708d9e06f615d5fb3dde54572c9bb2ea13550bfc

Comment 13

[Dana Keeler (she/her) (use needinfo) [:keeler]]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #8)
> ::: security/manager/ssl/tests/unit/pkcs11testmodule/pkcs11testmodule.cpp:122
> (Diff revisions 1 - 3)
> >          pSlotList[0] = 1;
> >          pSlotList[1] = 2;
> >          pSlotList[2] = 3;
> >          break;
> >        default:
> > -        // how did we get here?!?
> > +        MOZ_ASSERT_UNREACHABLE("Unexpected slot count in Test_C_GetSlotList");
> 
> So this would just be 'assert("Unexpected slot...");'

Whoops - heh. The vanilla assert just takes an expression that it expects to be true, so this should be `assert(false);` If we ever encounter it, we'll get the source line and it'll be pretty obvious what's up (particularly if we add a comment).

Comment 14

[Wouter Verhelst :wouter]

Comment on attachment 8919349 [details]
Bug 1404421 - Add an empty slot to the test PKCS#11 module

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/190208/diff/5-6/

Comment 15

[Wouter Verhelst :wouter]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #13)
> (In reply to David Keeler [:keeler] (use needinfo?) from comment #8)
> > So this would just be 'assert("Unexpected slot...");'
> 
> Whoops - heh. The vanilla assert just takes an expression that it expects to
> be true, so this should be `assert(false);` If we ever encounter it, we'll
> get the source line and it'll be pretty obvious what's up (particularly if
> we add a comment).

Yeah, I'd noticed that. I had wanted to do assert("Unexpected..." == NULL), which would have been syntactically valid (and still would've given the string), but then forgot to add the == NULL bit. Did that now.

eslint errors were fixed now, too.

Please check in again.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/70d574308366
Add an empty slot to the test PKCS#11 module r=keeler

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/70d574308366