Add an empty slot to the test PKCS#11 module

RESOLVED FIXED in Firefox 58

Status

()

P3
enhancement
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: wouter, Assigned: wouter)

Tracking

unspecified
mozilla58
Points:
---

Firefox Tracking Flags

(firefox58 fixed)

Details

(Whiteboard: [psm-backlog])

Attachments

(1 attachment)

(Assignee)

Description

a year ago
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.
Blocks: 1357391
Severity: normal → enhancement
Priority: -- → P3
Whiteboard: [psm-backlog]
Comment hidden (mozreview-request)
(Assignee)

Comment 2

a year ago
anyone care to review that?
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 hidden (mozreview-request)
(Assignee)

Comment 5

a year ago
mozreview-review-reply
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.
(Assignee)

Comment 6

a year ago
(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 hidden (mozreview-request)
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...");'
Attachment #8919349 - Flags: review?(dkeeler) → review+
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Keywords: checkin-needed
Assignee: nobody → w
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.
Flags: needinfo?(w)
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Keywords: checkin-needed
Comment hidden (mozreview-request)
(Assignee)

Comment 12

a year ago
(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
(Assignee)

Updated

a year ago
Flags: needinfo?(w)
(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 hidden (mozreview-request)
(Assignee)

Comment 15

a year ago
(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.
Keywords: checkin-needed

Comment 16

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/70d574308366
Add an empty slot to the test PKCS#11 module r=keeler
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/70d574308366
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.