284366 - PSM needs to handle Smart Cards seamlessly

Status: RESOLVED FIXED

(Core Graveyard :: Security: UI, defect)

Description

[Robert Relyea]

Firefox, thunderbird, and Mozilla, all already handle smart cards pretty
reasonably. They properly clear ssl sessions out when cards are removed, look up
certs if the card exists, etc.

What they don't do is automatically act when the state of the smart card has
changed. The following patch solves this problem.

NOTE: this is a first cut at the patch, it depends on the DOM, and may be doing
Bad Things(TM). Before this patch is applied, the architecture needs to be reviewed.

Also note: this patch includes changes to the idl for they crypto object. It
does not include the funny 'Undecryptable Object' UI that our demo had in
Thunderbird. I'll create a separate bug for that

Comment 1

[Robert Relyea]

Attachment: Smart card patch to PSM (and the crypto IDL).

This patch has been successfully applied to firefox PR1 , firefox 1.0,
thunderbird 0.8, thunderbird 0.9 and thunderbird 1.0. The only cavaet is the
platforms need to be brought up to NSS 3.9.3 or later. It's pretty likely to
apply to most mozilla based applications of the firefox vintage with little
modification;).

There are some advantages to PSM being 'stable';).

Comment 2

[Robert Relyea]

This patch needs adjustments to the copyright header.

Comment 3

[Robert Relyea]

Comment on attachment 175988 [details] [diff] [review]
Smart card patch to PSM (and the crypto IDL).

Marking as obsolete until the patch is corrected

Comment 4

[Robert Relyea]

Attachment: Updated patch

OK, this patch should be generally reviewable

Comment 5

[David :Bienvenu]

a few nits:
when you change an interface, you should rev the uuid.

 nsCrypto::nsCrypto()
 {
+  mNode = nsnull;
+  mRegistered = PR_FALSE;
 }

since mNode is an nsCOMPtr, you don't need to init it to nsnull.

+nsTokenEventRunnable::~nsTokenEventRunnable()
+{
+  if (mTokenName) {
+	free(mTokenName);

free already checks for null (there's some debate about this, but mailnews for
one won't work with a runtime where free doesn't check for null, and I doubt the
rest of mozilla will either)

Why drop rv here instead of just returning rv?

+{ 
+  nsresult rv;
+  nsCOMPtr<nsINSSComponent> nssComponent(do_GetService(kNSSComponentCID, &rv));
+  if (NS_FAILED(rv))
+    return NS_ERROR_FAILURE; 

@@ -221,6 +302,7 @@
   mTimer = nsnull;
   mCrlTimerLock = nsnull;
   mObserversRegistered = PR_FALSE;
+  mNodeList = NULL;

shouldn't NULL be nsnull?

+    while (mNodeList) {
+	delete mNodeList;
+    }
+

could this just be delete mNodeList? does the destructor have some strange side
effect of changing mNodeList?

+nsNSSComponent::DeregisterTarget(nsIDOMNode *aNode)
+{
+  nsresult rv = NS_ERROR_FAILURE;
+  nsNSSDOMNode *node;
+  nsAutoLock lock(mutex);
+  for (node = mNodeList; node; node = node->getNext()) {
+    if (node->getNode() == aNode) {
+      delete node;
+      node = NULL;
+      rv = NS_OK;
+      break;
+    }
+  }
+  return rv;

NULL -> nsnull, or why NULL out node since it's a local, and we're breaking out
of the loop anyway?

+  if (SECMOD_HasRemovableSlots(module)) {
+    if (mThreadList == NULL) {
+	mThreadList = new SmartCardThreadList();
+    }
+    mThreadList->Add(new SmartCardMonitoringThread(module));
+  }

should this be nsnull, or just if (!mThreadList)...

+SmartCardThreadList::~SmartCardThreadList()
+{
+   while (head) {
+	delete head;
+   }
+}

is this just to check for null? delete already checks for null.

there are a few other NULL's...

Comment 6

[Robert Relyea]

Attachment: Patch is 'openkeyinstall' removed

The openkeyinstall stuff was for our demo, not a proposal for inclusing (at
least for now) in the base mozilla.

Notes about this patch: I'm definately open to other ways of delivering the
events. Right now the proposal is to require 'pages' (including PSM xul pages)
that care about the events to register themselves to recieve the events. This
serves 2 purposes in the code 1) it gets us a handle to the doc, and 2) it
tells mozilla that it may need to initiate NSS to properly service this page
(if the page isn't already an SSL page).

I'm a little uncomfortable about 1) as there is nothing preventing the
javascript from sending *any* doc (not just the document in it's context). To
keep from creating circular dependencies, the PSM code does not create a
reference to the doc. it depends of the fact the the Crypto object will be
destroyed before the associated doc. Of course if the wrong doc is passed by
javascript....

Anyway that is way I'd like a security review, but most importantly a review
over the DOM stuff in the patch. jst suggested that it might be possible to
always generate the events and drop them through the top of the dom tree. In
this case, we wouldn't need to 'register' for events, and only pages that may
be started from some non-secure context (random webpages) would need to do
something to initialize nss.

bob

Comment 7

[Robert Relyea]

(In reply to comment #5)
> a few nits:
> when you change an interface, you should rev the uuid.

This includes adding? (This is the kind of input I need, I don't normally live
in the IDL/xpcom world of mozilla.
>
> 
> 
> free already checks for null (there's some debate about this, but mailnews for
> one won't work with a runtime where free doesn't check for null, and I doubt the
> rest of mozilla will either)

yeah, I'm paranoid. I'm pretty sure it fits with the style of the rest of PSM.

> 
> Why drop rv here instead of just returning rv?

Because it's a destructor?:) 
>
>    mObserversRegistered = PR_FALSE;
> +  mNodeList = NULL;
> 
> shouldn't NULL be nsnull?

Probably. My fingers are used to NSSism (nsnull does not exist in NSS code), so
there's probably a fair amount of those that may need to be cleaned up. I'm not
sure how pervasive NULL's are in PSM, though probably not to pervasive. PSM
usually conforms to mozilla norms more than NSS (since it's more tightly coupled
to mozilla).
> 
> +    while (mNodeList) {
> +	delete mNodeList;
> +    }
> +
> 
> could this just be delete mNodeList? does the destructor have some strange side
> effect of changing mNodeList?

Nodes are self-linking/removing lists, so this loop removes all the nodes on the
list.
> 
> +nsNSSComponent::DeregisterTarget(nsIDOMNode *aNode)
> +{
> +  nsresult rv = NS_ERROR_FAILURE;
> +  nsNSSDOMNode *node;
> +  nsAutoLock lock(mutex);
> +  for (node = mNodeList; node; node = node->getNext()) {
> +    if (node->getNode() == aNode) {
> +      delete node;
> +      node = NULL;
> +      rv = NS_OK;
> +      break;
> +    }
> +  }
> +  return rv;
> 
> NULL -> nsnull, or why NULL out node since it's a local, and we're breaking out
> of the loop anyway?

paranoia. In general, if it doesn't cost anything, I prefer to clean out
pointers I just freed. If new code is latter added below the loop, it won't wind
up with a bad pointer. This kind of paranoia isn't good deep inside some
function which could be called inside a tight loop (even little things add up
there), but if that applies to this function, then I'd look at changing the list
to a hashtable first;).
> 
> +SmartCardThreadList::~SmartCardThreadList()
> +{
> +   while (head) {
> +	delete head;
> +   }
> +}
> 
> is this just to check for null? delete already checks for null.

No it's a loop.

> 
> there are a few other NULL's...
> 
> 
Ok.

Comment 8

[David :Bienvenu]

>> when you change an interface, you should rev the uuid.

>This includes adding?

Yeah, even though it will usually not break c++ callers when you add methods to
the end of the vtable.

Why drop rv here instead of just returning rv?

+{ 
+  nsresult rv;
+  nsCOMPtr<nsINSSComponent> nssComponent(do_GetService(kNSSComponentCID, &rv));
+  if (NS_FAILED(rv))
+    return NS_ERROR_FAILURE; 

if it was a destructor, you wouldn't be returning NS_ERROR_FAILURE :-)  All I
meant was, either use

NS_ENSURE_SUCCESS(rv, rv); // this dumps a warning...

or if (NS_FAILED(rv))
      return rv; // instead of NS_ERROR_FAILURE

re NULL, I would never accidentally use NULL instead of nsnull, of course, but I
had a friend who did, and iirc, it didn't compile on some platforms :-)

Comment 9

[timeless]

bienvenu: we're still enforcing free not allowing null, i don't know where you
see code in mail that ddoesn't folow this rule, but i suspect it isn't along
critical paths.

+struct CryptoRunnableEvent : PLEvent {
+  CryptoRunnableEvent(nsIRunnable* runnable);
+  ~CryptoRunnableEvent();
+
+   nsIRunnable* mRunnable;
+};

please use 2 space indentation
please don't use tabs

+// QueryInterface implementation for nsCryptoRunnable
+NS_INTERFACE_MAP_BEGIN(nsTokenEventRunnable)
+  NS_INTERFACE_MAP_ENTRY(nsIRunnable)
+  NS_INTERFACE_MAP_ENTRY(nsISupports)
+NS_INTERFACE_MAP_END_THREADSAFE
+
+NS_IMPL_THREADSAFE_ADDREF(nsTokenEventRunnable)
+NS_IMPL_THREADSAFE_RELEASE(nsTokenEventRunnable)

you could just use
NS_IMPL_THREADSAFE_ISUPPORTS1(nsTokenEventRunnable, nsIRunnable)

+		new nsNSSDOMNode(aNode, mNodeList, NULL, &mNodeList);
+   return node ? NS_OK : NS_ERROR_FAILURE;
please return NS_ERROR_OUT_OF_MEMORY when allocs fail.

+  nsTokenEventRunnable *runnable = new nsTokenEventRunnable(eventType,token);
+
+  rv = nsNSSEventPostToUIEventQueue(runnable);
generally posting null doesn't work well and doesn't do what people want,
(certainly historically the previous things to do this were unhappy about
posting null), check for alloc failures.

+	mThreadList = new SmartCardThreadList();
+    }
+    mThreadList->Add(new SmartCardMonitoringThread(module));
null check your allocs!

+++ security/manager/ssl/src/nsNSSEvent.cpp	6 Apr 2005 23:43:55 -0000
is that really (c) netscape 2001?

+nsIEventQueue* 
+nsNSSEventGetUIEventQueue()
i wonder if this should return an alreadyAddRef'd

+  nsCOMPtr<nsIEventQueue>uiQueue = dont_AddRef(nsNSSEventGetUIEventQueue());
yuck. see above.

+nsNSSEventRunnable::nsNSSEventRunnable(nsIRunnable* runnable)
+  :  mRunnable(runnable)
+{
+  NS_ADDREF(mRunnable);
why aren't you using a comptr?

+void SmartCardMonitoringThread::Start()
Why don't you allow start to report failure (thread creation failure, this
really does happen).

Comment 10

[Robert Relyea]

Thanks for the comments...

> 
> please use 2 space indentation
> please don't use tabs

Tabs are the standard in the PSM directory (inheritted from NSS). Much of the
style in PSM is a mix of mozilla and NSS, so there will be a natural tension
here. Many of your comments below apply to mozilla style. They are applicable to
 this patch (places where the patch is less mozilla style than it should be).

One thing I would like to see is comments on the actual design of this patch,
not necessarily things like missing null pointer checks (though I'll take those
as well since they have to be fixed). What I'm worried about is the overall
plan. (see comment 6 for more of what I'm looking for). 

bob

Comment 11

[Robert Relyea]

Attachment: Smart Cards 2.0

This patch does 2 thinks over the last one:

1) it incorporates a design I discussed with jst for how this should work in
mozilla in general.
2) it incorporates the other feedback I got from dennis and timeless.

Note, PSM is a mix of NSS style and mozilla style conventions, however the
existing files are mostly mozilla style, and this patch attempts to emulate
that. Places where it converges (including if you find any tabs in the patch;).
should be flagged.

Now it's ready for a general review. (I hope).

bob

Comment 12

[Robert Relyea]

Comment on attachment 181371 [details] [diff] [review]
Smart Cards 2.0

move review request to latest patch.

Comment 13

[timeless]

Comment on attachment 181371 [details] [diff] [review]
Smart Cards 2.0

>Index: dom/public/idl/events/nsIDOMSmartCardEvent.idl
>@@ -0,0 +1,47 @@
>+/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* ***** BEGIN LICENSE BLOCK *****
new files are generally supposed to be mpl not npl:
>+ * Version: NPL 1.1/GPL 2.0/LGPL 2.1

>+ * The Original Code is mozilla.org code.
i don't suppose you could be more specific? :)

is this block correct:
>+ * The Initial Developer of the Original Code is 
>+ * Netscape Communications Corporation.
>+ * Portions created by the Initial Developer are Copyright (C) 2000
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Johnny Stenback <[e-mail]> (original author)
>+ *   Bob Relyea

>Index: security/manager/pki/resources/content/certManager.js
>@@ -48,6 +48,10 @@
> 
> function LoadCerts()
> {
>+  window.crypto.enableSmartCardEvents = true;
>+  document.addEventListener("smartcard-insert",onSmartCardChange,false);
spaces after commas :)
>+  document.addEventListener("smartcard-remove",onSmartCardChange,false);

>@@ -432,6 +436,23 @@
>   }
> }
> 
>+function onSmartCardChange()
>+{
>+  var certcache = Components.classes[nsNSSCertCache].createInstance(nsINSSCertCache);
>+  // We've change the state of the smart cards inserted or removed
// The state of the smart cards has changed, either (?) inserted or removed

>+  // that means the available certs may have changed. Update the display
>+  certcache.cacheAllCerts();
note that you put spaces after commas here: =)
>+  userTreeView.loadCertsFromCache(certcache, nsIX509Cert.USER_CERT);
none of these methods can trigger exceptions? (note that it is possible for any
method call to trigger an exception) you don't need/want to /try/ to do the
other loads if the earlier ones fail?
>+  userTreeView.selection.clearSelection();
>+  caTreeView.loadCertsFromCache(certcache, nsIX509Cert.CA_CERT);
>+  caTreeView.selection.clearSelection();
>+  serverTreeView.loadCertsFromCache(certcache, nsIX509Cert.SERVER_CERT);
>+  serverTreeView.selection.clearSelection();
>+  emailTreeView.loadCertsFromCache(certcache, nsIX509Cert.EMAIL_CERT);
>+  emailTreeView.selection.clearSelection();

>Index: security/manager/pki/resources/content/device_manager.js
>@@ -41,8 +41,18 @@
> {
>   bundle = srGetStrBundle("chrome://pippki/locale/pippki.properties");
>   secmoddb = Components.classes[nsPKCS11ModuleDB].getService(nsIPKCS11ModuleDB);
>+  window.crypto.enableSmartCardEvents = true;
>+  document.addEventListener("smartcard-insert",onSmartCardChange,false);
spaces please :) [last time i'll comment on this, the patch is large...]
>+  document.addEventListener("smartcard-remove",onSmartCardChange,false);
>+
>+  RefreshDeviceList();
>+}

>+function ClearDeviceList()
>+{
>+  //Remove the existing listed modules so that re-fresh doesn't 
why the hyphen?
maybe add a space after // to match your other comments (ok, the code isn't
consistent)
>+  //display the module that just changed.
>+  var device_list = document.getElementById("device_list");
>+  while (device_list.firstChild)
>+    device_list.removeChild(device_list.firstChild);
>+}
>+
extra blank line:
>+
>@@ -349,13 +373,23 @@
>+// handle card insertion an removal

>Index: security/manager/ssl/src/nsCrypto.cpp

> nsCrypto::nsCrypto()
can you use initialization style? : mEnableSmartCardEvents(PR_FALSE)
> {
>+  mEnableSmartCardEvents = PR_FALSE;
> }

>+  // already going. nssComponent starting is a prerequisit for starting
Did you mean: prerequisite

>+  if (aEnable == PR_TRUE) {
normally we do if (PRBool) not if (PRBool == PR_TRUE) ...
>+    nsCOMPtr<nsINSSComponent> nssComponent(do_GetService(kNSSComponentCID, &rv));
>+  }
else rv = PURIFY_ERROR_NOT_INITIALIZED_COMPLAIN_TO_CODER // don't do this ;-)
>+  if (NS_FAILED(rv)) {
>+    return rv;
>+  }
>+nsCrypto::GetEnableSmartCardEvents(PRBool *aEnable)
>+{
>+  *aEnable = mEnableSmartCardEvents;
>+  return NS_OK;
> }
> 
>+
is double space after functions some style?

>Index: security/manager/ssl/src/nsNSSComponent.cpp
>@@ -205,8 +217,43 @@
>+//This class is used to run the callback code
>+//passed to the event handlers for smart card notification
>+class nsTokenEventRunnable : public nsIRunnable {
don't do this:
>+  NS_IMETHOD Run ();
>+  NS_DECL_ISUPPORTS
add NS_DECL_IRUNNABLE

> nsNSSComponent::nsNSSComponent()
>+:mNSSInitialized(PR_FALSE), mThreadList(nsnull)
> {
>   mutex = PR_NewLock();
does anything ensure mutex (PR_NewLock can fail...)

>@@ -264,6 +311,145 @@
your use of rrv doesn't match the normal use, generally we'd have an rv2 or
something which is used for the inner stuff, and then assign that to rv.
>+  nsresult rrv = NS_OK;
>+
>+  while (NS_SUCCEEDED(enumerator->HasMoreElements(&hasMoreWindows))
>+      nsresult rv = DispatchEventToWindow(domWin, eventType, tokenName);
>+        rrv = rv;
>+  return rrv;

>+nsNSSComponent::DispatchEventToWindow(nsIDOMWindow *domWin,
>+                      const nsAString &eventType, const nsAString &tokenName)
>+{
>+  // first walk the children and dispatch their events 
>+  {
>+    nsCOMPtr<nsIDOMWindowCollection> frames;
>+    domWin->GetFrames(getter_AddRefs(frames));
please check the rv for this function, while it happens that the current impl
of GetFrames actually outs this and always returns NS_OK, you might some day be
given a proxy (dconnect, timeless-wrong-threaded-window) where you encounter an
oom and the proxy has to return NS_ERROR_OUT_OF_MEMORY.
>+    PRUint32 length;
>+    frames->GetLength(&length);
>+    PRUint32 i;
>+    for (i = 0; i < length; i++) {
>+      nsCOMPtr<nsIDOMWindow> childWin;
>+      frames->Item(i, getter_AddRefs(childWin));
>+      DispatchEventToWindow(childWin, eventType, tokenName);
>+    }
>+  }

>+  // find the document
>+  nsCOMPtr<nsIDOMDocument> doc;
>+  rv = domWin->GetDocument(getter_AddRefs(doc));
as it happens, this tends to return ns_ok + out = null
>+  if (NS_FAILED(rv)) {
>+    return rv;
>+  }

>+  // create the event
which means you'll get ns_error_null_pointer here:
>+  nsCOMPtr<nsIDOMDocumentEvent> docEvent = do_QueryInterface(doc, &rv);
>+  if (NS_FAILED(rv)) {
>+    return rv;
>+  }

>+  rv=docEvent->CreateEvent(NS_LITERAL_STRING("Events"), getter_AddRefs(event));
spaces around =

>@@ -333,6 +519,59 @@
> }
> 
> void
>+nsNSSComponent::LaunchSmartCardThreads()
>+{
>+  nsNSSShutDownPreventionLock locker;
>+  {
>+    SECMODModuleList *list = SECMOD_GetDefaultModuleList();
i'm guessing this can fail:
>+    SECMODListLock *lock = SECMOD_GetDefaultModuleListLock();
and that this will crash:
>+    SECMOD_GetReadLock(lock);
please be nice to your poor callers.

>+NS_IMETHODIMP
>+nsNSSComponent::LaunchSmartCardThread(SECMODModule *module)
>+{

>+    if (mThreadList == nsnull) {
you didn't check this for failure:
>+      mThreadList = new SmartCardThreadList();
>+    }
>+    newThread = new SmartCardMonitoringThread(module);
>+    if (!newThread) {
>+	return NS_ERROR_OUT_OF_MEMORY;
>+    }
>+    // newThread is adopted by the add.
so you'll crash here:
>+    return mThreadList->Add(newThread);

>+nsNSSComponent::ShutdownSmartCardThread(SECMODModule *module)
>+{
>+  if (!mThreadList) {
indentation is wrong:
>+     return NS_OK;
>+  }
>+  mThreadList->Remove(module);

more after lunch starting near:
>+nsNSSComponent::ShutdownSmartCardThreads()

Comment 14

[timeless]

Comment on attachment 181371 [details] [diff] [review]
Smart Cards 2.0

>@@ -598,9 +841,9 @@
should we cache the return from CERT_GetDefaultCertDB()?
>+    CERT_DisableOCSPChecking(CERT_GetDefaultCertDB());
>+    CERT_DisableOCSPDefaultResponder(CERT_GetDefaultCertDB());

>@@ -637,17 +878,8 @@
>+  nsCOMPtr<nsIEventQueue>uiQueue = nsNSSEventGetUIEventQueue();
this can return null
> 
>   //Post the event
and then you crash:
>   return uiQueue->PostEvent(event);

>Index: security/manager/ssl/src/nsPK11TokenDB.cpp
>@@ -45,8 +45,16 @@
>+  mUIContext = new PipUIContext();
do things handle this?

>+nsPK11Token::refreshTokenInfo()
>+  mTokenName = NS_ConvertUTF8toUCS2(PK11_GetTokenName(mSlot));
use CopyUTF8toUTF16

>Index: security/manager/ssl/src/nsPKCS11Slot.cpp
>@@ -76,10 +82,12 @@
>+    mSlotHWVersion = NS_LITERAL_STRING("");
use EmptyString()
>     mSlotHWVersion.AppendInt(slot_info.hardwareVersion.major);
hrm, perhaps we should have NS_NAMED_LITERAL_STRING(period, "."); or perhaps
use AppendLiteral(".") ...
>     mSlotHWVersion.Append(NS_LITERAL_STRING("."));
>     mSlotHWVersion.AppendInt(slot_info.hardwareVersion.minor);
>     // Set the Firmware Version field
>+    mSlotFWVersion = NS_LITERAL_STRING("");
>     mSlotFWVersion.AppendInt(slot_info.firmwareVersion.major);
>     mSlotFWVersion.Append(NS_LITERAL_STRING("."));
>     mSlotFWVersion.AppendInt(slot_info.firmwareVersion.minor);
>@@ -201,6 +222,16 @@
>   if (isAlreadyShutDown())
>     return NS_ERROR_NOT_AVAILABLE;
> 
>+  if (!PK11_IsPresent(mSlot)) {
mozilla code uses nsnull:
>+    *aName = NULL;
>+    return NS_OK;

>@@ -306,27 +337,31 @@
this pair of lines could be combined (yes, i know it isn't your code):
>   char *asciiname = NULL;
>   asciiname = ToNewUTF8String(nsDependentString(aName));
>   PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("Getting \"%s\"\n", asciiname));
>+  PK11SlotInfo *slotinfo = NULL;
>+  PK11SlotList *slotList = PK11_FindSlotsByNames(mModule->dllName, 
is PK11_FindSlotByNames happy w/ asciiname == 0 (ToNewUTF8String can fail)
>+        asciiname /* slotName */, NULL /* token Name */, PR_FALSE);

>+  if (!slotList) {
>+    /* name must be the token name */
>+    slotList = PK11_FindSlotsByNames(mModule->dllName, 
>+        NULL /*slot Name */, asciiname /* token Name */, PR_FALSE);
>+  }

i think we're starting to drop nsCRT (make sure asciiname isn't null if you do
so):
>+    if (nsCRT::strcmp(asciiname, "Root Certificates") == 0) {
>+      slotinfo = PK11_ReferenceSlot(mModule->slots[0]);

>@@ -351,12 +386,19 @@
>+  /* applications which allow new slot creation (which Firefox now does
>+   * since it uses the WaitForSlotEvent call) need to hold the
>+   * ModuleList Read lock to prevent the slot array from changing out
>+   * from under it. */
>+  SECMODListLock *lock = SECMOD_GetDefaultModuleListLock();
>+  SECMOD_GetReadLock(lock);
>   for (i=0; i<mModule->slotCount; i++) {
>     if (mModule->slots[i]) {
i presume this should be null checked (yes, i know it isn't your code):
>       nsCOMPtr<nsIPKCS11Slot> slot = new nsPKCS11Slot(mModule->slots[i]);
>       array->AppendElement(slot);

>@@ -466,6 +508,13 @@
>+  /* Get the modules in the database that didn't load */
>+  list = SECMOD_GetDeadModuleList();
>+  while (list) {
i presume this should be null checked (i.e. that you don't want to append
null):
>+    nsCOMPtr<nsIPKCS11Module> module = new nsPKCS11Module(list->module);
>+    array->AppendElement(module);
>+    list = list->next;
>+  }

>Index: security/manager/ssl/src/nsSmartCardEvent.cpp

>+nsSmartCardEvent::nsSmartCardEvent(nsIDOMEvent * aInner, 
>+                      const nsAString &aTokenName) : mTokenName(aTokenName)
>+{
mInner/mPrivate are members of this class, so use initialization : mInner(...),
mPrivate(...)
NS_ASSERTION(mInner, "...")?
>+  mInner = aInner; 
>+  mPrivate = do_QueryInterface(mInner);

this isn't the spelling of the interface:
>+// IPrivateDOMEvent maps
>+NS_IMETHODIMP nsSmartCardEvent::DuplicatePrivateData(void)
>+{
bad indentation:
>+   if (!mPrivate) {
>+    return NS_ERROR_NO_INTERFACE;
>+   }

>+NS_IMETHODIMP nsSmartCardEvent::SetCurrentTarget(nsIDOMEventTarget *aTarget)
>+{
is this really the right error?
>+   if (!mPrivate) {
>+    return NS_ERROR_NO_INTERFACE;

>+NS_IMETHODIMP nsSmartCardEvent::HasOriginalTarget(PRBool *aResult)
>+{
this is a different error?
>+   if (!mPrivate) {
>+    return NS_ERROR_FAILURE;

>+NS_IMETHODIMP nsSmartCardEvent::GetType(nsAString & aType)
>+{
see comment about asserting mInner (somewhere)
>+  return mInner->GetType(aType);
>+}

>Index: security/manager/ssl/src/nsSmartCardEvent.h

>+class nsSmartCardEvent : public nsIDOMSmartCardEvent,
>+                         public nsIPrivateDOMEvent
>+public:
>+  nsSmartCardEvent(nsIDOMEvent * aInner, const nsAString &aTokenName);
>+protected:
members that should be initialized :foo(style) instead of assignment in
constructor.
>+  nsCOMPtr<nsIDOMEvent> mInner;
>+  nsCOMPtr<nsIPrivateDOMEvent> mPrivate;
>+  nsString mTokenName;

>Index: security/manager/ssl/src/nsSmartCardMonitor.cpp

>+// The SmartCard monitoring thread should start up for each module we load
>+// that has removable tokens. This code calls an NSS function which waits
_that_ waits
>+// until there is a change in the token state. NSS uses the 
>+// C_WaitForSlotEvent() call in PKCS #11 if  the token implements the call,
random double space between 'if  the'	     ^^
>+// otherwise NSS will pull the token in a loop with a delay of 'latency' 
pull or poll?
>+// insertion and removal events we are looking for.
for which we are looking :) - don't end sentences with prepositions.

yuck:
>+#include <assert.h>

>+// self linking an removing double linked entry
'an' ?? do you mean 'a' or 'and'?

>+// That way new thread could be started and old ones terminated as we
'a new thread'?
>+// load and unload modules.

>+  // the is is self linking and unlinking, the following
is is?

this code is (and was) evil, but i understand that it works :)
>+  while (head) {
>+    delete head;
>+  }

>+      // NOTE: automatically stops the thread and dequeues
dequeues _what_? :)
>+      delete current;

>+// adopts the thread passwd to it. Starts the thread as well
'passed'

>+SmartCardThreadList::Add(SmartCardMonitoringThread *thread)
>+{
>+  SmartCardThreadEntry *current = new SmartCardThreadEntry(thread, head, nsnull,
>+                                                           &head);
normally we return early on error
>+  if (current) {  
>+     // OK to forget current here, it's on the list
>+    return thread->Start();
>+  }
>+  return NS_ERROR_OUT_OF_MEMORY;

>+// We really should have a Unity PL Hash function...
is this a XXX missing a bug number, or just a generic random comment? :)

>+SmartCardMonitoringThread::Start()
>+{
>+  if (!mThread) {
>+    mThread = PR_CreateThread(PR_SYSTEM_THREAD, LaunchExecute, this,
>+                              PR_PRIORITY_NORMAL, PR_LOCAL_THREAD,
>+                              PR_JOINABLE_THREAD, 0);
>+  }
i'd rather NS_ERROR_OUT_OF_MEMORY over generic failure...
>+  return mThread ? NS_OK : NS_ERROR_FAILURE;

>+// Should only stop if we are through with the module.
>+// CancelWait has the side effect of losing all the keys and
'losing'?

>+    PR_JoinThread(mThread);
i don't understand the ownership model here, please add a comment to explain
why this is right or fix it :)
>+    mThread = 0; 

>+// remember the name and series of a token in a particular slot.
>+// This is important because the name is no longer available when
>+// the token is removed. If listeners depended on this information,
>+// They would be out of luck. It also is a handy way of making sure
'they'
>+// we don't generate spurious insertion and removal events as the slot
>+// cycles through various states.

>+SmartCardMonitoringThread::SetTokenName(CK_SLOT_ID slotid, 
>+                                       const char *tokenName, PRUint32 series)
		  this is under indented ^ by one space.

spaces around '=' and ',' and '+':
>+      int len=strlen(tokenName)+1;
>+      char *entry= (char *)malloc(len+sizeof(PRUint32));
>+     
>+      if (entry) {  
>+        memcpy(entry,&series,sizeof(PRUint32));
>+        memcpy(&entry[sizeof(PRUint32)],tokenName,len);
>+
>+        PL_HashTableAdd(mHash,(void *)slotid, entry); /* adopt */
>+        return;

>+SmartCardMonitoringThread::GetTokenSeries(CK_SLOT_ID slotid)
>+    entry = (const char *)PL_HashTableLookupConst(mHash,(void *)slotid);
>+      memcpy(&series,entry,sizeof(PRUint32));

>+SmartCardMonitoringThread::SendEvent(const nsAString &eventType,
>+                                                         const char *tokenName)
this is overindented by quite a few spaces ^^^^^^^^^^^^^^^^

personally i do:
for (foo = bar;
     baz;
     bleh) {
>+    for (sle=PK11_GetFirstSafe(sl); sle; 
>+                                      sle=PK11_GetNextSafe(sl,sle,PR_FALSE)) {
>+      SetTokenName(PK11_GetSlotID(sle->slot), 
>+                 PK11_GetTokenName(sle->slot), PK11_GetSlotSeries(sle->slot));
>+    }

>+    // now we have a potential insertion or removal event see if the slot
now that we ... event, see [i think, someone can correct me if i'm wrong]

i believe this isn't legal (i suspect the macros don't get along in certain
cases, esp on windows):
>+          SendEvent(NS_LITERAL_STRING(SMARTCARDEVENT_REMOVE), tokenName);
>+        SendEvent(NS_LITERAL_STRING(SMARTCARDEVENT_INSERT), tokenName);
>+        SendEvent(NS_LITERAL_STRING(SMARTCARDEVENT_REMOVE), tokenName);
that said, you'd probably want to consider a named literal string if the macros
did get along.

all done.

Comment 15

[Johnny Stenback (:jst)]

Comment on attachment 181371 [details] [diff] [review]
Smart Cards 2.0

- In nsIDOMClassInfo.h:

@@ -80,6 +80,7 @@
   // Event classes
   eDOMClassInfo_Event_id,
   eDOMClassInfo_MutationEvent_id,
+  eDOMClassInfo_SmartCardEvent_id,

This should be at the end of the list of classinfo id's, right before
eDOMClassInfoIDCount.

- In nsIDOMSmartCardEvent.idl:

+ * Contributor(s):
+ *   Johnny Stenback <[e-mail]> (original author)
+ *   Bob Relyea

Feel free to leave me out of the list of contributors here and list yourself as
the original author (and if you for whatever reason don't want to do that then
at least change my email address to [e-mail]).

- In nsGlobalWindow.cpp:

+    // clear smartcard events, our document has gone away.
+    if (mCrypto) {
+	mCrypto->SetEnableSmartCardEvents(PR_FALSE);
+    }

Beware that how this works may change when bug 274784 is fixed (should land any
day now). It should be pretty straight forward to make this code do the same
after that change has gone in.

- In nsNSSComponent::LaunchSmartCardThread():

+    if (mThreadList == nsnull) {
+      mThreadList = new SmartCardThreadList();
+    }
+    newThread = new SmartCardMonitoringThread(module);
+    if (!newThread) {
+	return NS_ERROR_OUT_OF_MEMORY;
+    }
+    // newThread is adopted by the add.
+    return mThreadList->Add(newThread);

mThreadList isn't checked for out of memory here, add a null check after
allocating it.

+nsSmartCardEvent::nsSmartCardEvent(nsIDOMEvent * aInner, 
+		       const nsAString &aTokenName) : TokenName(aTokenName)
+{
+  mInner = aInner; 
+  mPrivate = do_QueryInterface(mInner);
+}
[...]
+// IPrivateDOMEvent maps
+NS_IMETHODIMP nsSmartCardEvent::DuplicatePrivateData(void)
+{
+   if (!mPrivate) {
+    return NS_ERROR_NO_INTERFACE;
+   }
+
+   return mPrivate->DuplicatePrivateData();
+}

It'd be cool if you'd added an Init() method, or a static Create() method or
somesuch that would move the null check of mPrivate out of every method where
you use it into a single place. A DOM event not QI'able to nsIPrivateDOMEvent
would be reason enough not to dispatch the event at all.

And this Init() or Create() method woukld be a good place to put a call to
mPrivate->SetTrusted(PR_TRUE) to flag that this event is a trusted (i.e. not a
script creted synthetic event from a webpage). You'll need this now that bug
289940 is fixed.

And fix the 3/1 space indentation in all the methods where you use mPrivate.

- In SmartCardMonitoringThread::Execute():

[...]
+      // retrieve token name 
+      tokenName = GetTokenName(PK11_GetSlotID(slot));
+      // if there's not a token name, then the software isn't expecting
+      // a (or another) remove event.
+      if (tokenName) {
+	 // clear the token name
+	 SetTokenName(PK11_GetSlotID(slot), nsnull, 0);
+	 SendEvent(NS_LITERAL_STRING(SMARTCARDEVENT_REMOVE), tokenName);

tokenName is pointing into deleted memory here, it's deleted when its removed
from the hash in the above SetTokenName() call. And you might want to cache the
result of PK11_GetSlotID(slot) in a local here (unless the call is effectively
free).

sr=jst with that fixed.

Comment 16

[Robert Relyea]

Attachment: Incorporate timeless and jst changes.

Comment 17

[Robert Relyea]

This patch  handles most of timeless comments (some of the javascript stuff that
was pre-existing wasn't touched), and all but one of jst's comments.

the one jst comment was the interaction with bug 274784. If this never gets
fixed, the only result is smart-card events could be issued to documents which
have not requested them (if they live in the same window as a previous document
that requested the events). since the new documents do not have handlers
registered for these events they will be ignored.

The risk is low for most users. Even though the patch is relatively large, it
only affects code with is triggered when smart cards are installed. There is a
medium risk to smart card users, though most users should have a better smart
card experience (a number of bugs relating to displaying smart card certs and
data have also been fixed).

Comment 18

[Robert Relyea]

Comment on attachment 183514 [details] [diff] [review]
Incorporate timeless and jst changes.

This patch was reviewed by jst and received an sr+ with comments (the patch
incorporates the comments).

It only affects smart card users.

Comment 19

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 183514 [details] [diff] [review]
Incorporate timeless and jst changes.

2xa=shaver

Comment 20

[Christian Persch (GNOME) (away; not receiving bug mail)]

I'd like to note that timeless' concern about licensing in comment 13 wasn't
addressed: nsIDOMSmartCardEvent.idl, nsSmartCardEvent.h, and
nsSmartCardEvent.cpp are NPL/GPL/LGPL, and nsNSSEvent.cpp is MPL-only.

Comment 21

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Hrm, indeed.  Bob: can you commit a fast followup to this to fix the licensing,
r+sr+a=shaver?  Thanks!

Comment 22

[Robert Relyea]

Yikes, I fixed nsSmartCardMonitor* and missed the others.
Applying patches  now.

Comment 23

[Robert Relyea]

The code is in marking fixed

Comment 24

[Wolfgang Rosenauer [:wolfiR]]

please note that this code breaks with gcc4 on Linux because:

nsSmartCardMonitor.cpp: In function 'PLHashNumber unity(const void*)':
nsSmartCardMonitor.cpp:134: error: cast from 'const void*' to 'PLHashNumber'
loses precision

Comment 25

[Robert Relyea]

I'm willing to review a patch. The 'unity' function takes a  pointer and returns
a hash bucket value. It's perfectly the code would be perfectly happy with an
implementation that purposefully truncates bits (preferably high bits). It's OK
for the function to have colisions (that is it's OK for 0xe123456712345678 to
return the same as 0xd765432112345678). We just don't want 'frequent' colisions
(so always returning '1' would work, but wouldn't be optimal).

bob

Comment 26

[Andrew Schultz]

Attachment: fix casting on 64bit platforms

Comment 27

[Johnny Stenback (:jst)]

Comment on attachment 185841 [details] [diff] [review]
fix casting on 64bit platforms

sr=jst

Comment 28

[Robert Relyea]

Comment on attachment 185841 [details] [diff] [review]
fix casting on 64bit platforms

r+ relyea

Comment 29

[Andrew Schultz]

Comment on attachment 185841 [details] [diff] [review]
fix casting on 64bit platforms

simple 64bit fix

Comment 30

[Andrew Schultz]

Comment on attachment 185841 [details] [diff] [review]
fix casting on 64bit platforms

this is in

Comment 31

[timeless]

Comment on attachment 185841 [details] [diff] [review]
fix casting on 64bit platforms

mozilla/security/manager/ssl/src/nsSmartCardMonitor.cpp 	1.3