388128 - [FIX]Make it possible to fastload nsNSSCertificates

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

Need to implement nsISerializable and nsIClassInfo.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Like so

Comment 2

[Brendan Eich [:brendan]]

Are all those classinfo stubs required? Or are there shared generic stubs you could use (or should these be non-stubs that return useful results)?

/be

Comment 3

[Boris Zbarsky [:bzbarsky]]

> Are all those classinfo stubs required?

Yes, because fastload needs the CID.  It's really a pain in the behind.

> Or are there shared generic stubs you could use 

There aren't.  I almost considered creating some macros for this....

> (or should these be non-stubs that return useful results)

They could be if other consumers want to use them, but not needed for fastload.

Comment 4

[Brendan Eich [:brendan]]

(In reply to comment #3)
> > Are all those classinfo stubs required?
> 
> Yes, because fastload needs the CID.  It's really a pain in the behind.

Something here is. My view used to be that lack of universal classinfo was the source of pain. But I'm now inclined to think all XPCOM must die. :-|

> > Or are there shared generic stubs you could use 
> 
> There aren't.  I almost considered creating some macros for this....

Cc'ing :bs.

/be

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 272504 [details] [diff] [review]
Like so

Kai, you first -- I try to sr after the r+.

/be

Comment 6

[Benjamin Smedberg]

Comment on attachment 272504 [details] [diff] [review]
Like so

Tabs crept into this patch.

Yes, macros would be nice. Yes, making serious changes to XPCOM while being source-code-compatible-or-autorewritable would be even better.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Ugh.  I'll fix the tabs.  Darned missing modelines!

Comment 8

[Kai Engert (:KaiE:)]

If guess it's not possibly to make the new constructor
>  nsNSSCertificate::nsNSSCertificate();
protected or private?
I'd prefer that if possibly, but I guess you need it public for the serialization to work?


> +  if (!!InitFromDER(const_cast<char*>(str.get()), len)) {

I guess you don't want 2 exclamation marks?


in both
> +nsNSSCertificate::Write(nsIObjectOutputStream* aStream)
> +nsNSSCertificate::Read(nsIObjectInputStream* aStream)

Should you check that aStream is non-NULL before you dereference it?



> +nsNSSCertificate::Read(nsIObjectInputStream* aStream)

What happens if you call this on a instance that got already initialized?
I guess it's unlikely, but for completeness I'd prefer a cleanup like:
  if (mCert) {
    destroy old cert
    set mCert/type/permdelete to initial values
      (should reading or initFromDER fail)
  }


> +  // XXXbz and if CERT_DupCertificate fails?

Then mCert == nsnull, which should be ok.
(I realize that some methods are dereferencing mCert without checking, but that's another bug. At least this will always be a good crash.)


> +nsNSSCertificate::GetInterfaces(PRUint32 *count, nsIID * **array)
> +nsNSSCertificate::GetHelperForLanguage(PRUint32 language, nsISupports **_retval)
> +nsNSSCertificate::GetContractID(char * *aContractID)
> +nsNSSCertificate::GetClassDescription(char * *aClassDescription)
> +nsNSSCertificate::GetClassID(nsCID * *aClassID)
> +nsNSSCertificate::GetImplementationLanguage(PRUint32 *aImplementationLanguage)
> +nsNSSCertificate::GetFlags(PRUint32 *aFlags)
> +nsNSSCertificate::GetClassIDNoAlloc(nsCID *aClassIDNoAlloc)

Should you use NS_ENSURE_ARG on the pointer params?


> +nsNSSCertificate::GetFlags(PRUint32 *aFlags)

Out of curiousity, is there a special reason why you limit this to the main thread only?


I'll review quickly when you attach a new patch.

Comment 9

[Kai Engert (:KaiE:)]

cc'ing Bob FYI only

Comment 10

[Boris Zbarsky [:bzbarsky]]

> If guess it's not possibly to make the new constructor protected or private?

Hmm... I could try to make it private and make the factory constructor a friend function so that it can still call it.  I'll do that.

> I guess you don't want 2 exclamation marks?

Yikes.  No, I do not.

> Should you check that aStream is non-NULL before you dereference it?

That would be a contract violation for this interface.  If you prefer, I can null-check it anyway, of course.  Please let me know.

> What happens if you call this on a instance that got already initialized?

I'd be happy to either do the cleanup or make that case throw.  I'd prefer throwing, I think.  Again, let me know which you prefer.

> Should you use NS_ENSURE_ARG on the pointer params?

In general I would not for XPCOM out params.  But it's your module; if you prefer I can do that.

> Out of curiousity, is there a special reason why you limit this to the main
> thread only?

Um... no.  That's a copy/paste from another class that I had with similar classinfo.  If this class is ok off the main thread we should change that.  Given your question I assume it is.

Will attach new patch once you tell me what you want for some of the above items.

Comment 11

[Benjamin Smedberg]

You can make the factory function a static class function to avoid nasty friendship requirements:

class myClass {
  static NS_METHOD Create(...);
};

Comment 12

[Brendan Eich [:brendan]]

(In reply to comment #10)
> > Should you check that aStream is non-NULL before you dereference it?
> 
> That would be a contract violation for this interface.  If you prefer, I can
> null-check it anyway, of course.  Please let me know.

Shouldn't be necessary.

> > Should you use NS_ENSURE_ARG on the pointer params?
> 
> In general I would not for XPCOM out params.  But it's your module; if you
> prefer I can do that.


Should not be necessary.

> > Out of curiousity, is there a special reason why you limit this to the main
> > thread only?
> 
> Um... no.  That's a copy/paste from another class that I had with similar
> classinfo.  If this class is ok off the main thread we should change that. 
> Given your question I assume it is.

FastLoad is main thread only, FWIW.

/be

Comment 13

[Boris Zbarsky [:bzbarsky]]

> FastLoad is main thread only, FWIW.

Sure.  But nsNSSCertificates are used by things other than fastload.

Comment 14

[Kai Engert (:KaiE:)]

(In reply to comment #10)
> > If guess it's not possibly to make the new constructor protected or private?
> 
> Hmm... I could try to make it private and make the factory constructor a friend
> function so that it can still call it.  I'll do that.


Both your proposal and Benjamin's proposal sound good to me.


> > What happens if you call this on a instance that got already initialized?
> 
> I'd be happy to either do the cleanup or make that case throw.  I'd prefer
> throwing, I think.  Again, let me know which you prefer.


I like the idea to abort with a failure in ::Read if mCert is already set.


> > Out of curiousity, is there a special reason why you limit this to the main
> > thread only?
> 
> Um... no.  That's a copy/paste from another class that I had with similar
> classinfo.  If this class is ok off the main thread we should change that. 
> Given your question I assume it is.

Yes, the class is thread safe.


As both you and Brendan confirm checking the parameters for NULL isn't necessary, no need to add the checks.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

The constructor is still public, because if I make it private, the factory method will still be public.  So there's no net win.  If you want me to jump through the hoops needed to make it private anyway, do let me know, but I don't see a benefit to it...

Comment 16

[Kai Engert (:KaiE:)]

Comment on attachment 275549 [details] [diff] [review]
Updated to comments

(In reply to comment #15)
> The constructor is still public, because if I make it private, the factory
> method will still be public.  So there's no net win.  If you want me to jump
> through the hoops needed to make it private anyway, do let me know, but I don't
> see a benefit to it...

I don't want to ask you to do unnecessary extra work, I had hoped it to be simple.

My motivation was the initial design of the class, which didn't allow a way to construct an empty instance, but always required to pass in a cert.

But given the fact that one is able to pass NULL to the old constructor, which effectively constructs an empty instance, too, and we therefore need to be prepared for that scenario anyway, it seems acceptable to have this additional default constructor public.

I'm ok with this patch, r=kengert

FWIW, you have an unnecessary whitespace change in nsNSSCertificate::GetRawDER, but I don't mind.

(And I wish the NSS interfaces used const more often, so that your const_cast wouldn't be necessary :-/ )

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 275549 [details] [diff] [review]
Updated to comments

Requesting 1.9 approval.  This adds code to serialize/deserialize NSS certificates; something we need to serialize/deserialize principals (for session restore, e.g.).  Risk is very low.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 275549 [details] [diff] [review]
Updated to comments

It's been a week and a half.  Just self-approving, dammit.

Comment 19

[Boris Zbarsky [:bzbarsky]]

> FWIW, you have an unnecessary whitespace change in nsNSSCertificate::GetRawDER,

Checked in without that whitespace change.

Comment 20

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #3)
> > Or are there shared generic stubs you could use 
> 
> There aren't.  I almost considered creating some macros for this....

Hm? Why can't you use the normal classinfo helpers? (http://developer.mozilla.org/en/docs/Using_nsIClassInfo#I.27m_writing_C_code.2C_and_I_use_the_generic_factory_and_nsModuleComponentInfo.)

Comment 21

[Boris Zbarsky [:bzbarsky]]

Because it's actually more painful than implementing nsIClassInfo on the object directly?

Cristian caught a bug in this patch, by the way: I didn't change the QI impl!  Need to fix that.  Would be nice to have actual certs to write tests around...

Comment 22

[Christian :Biesinger (don't email me, ping me on IRC)]

patch in bug 405481 for that issue