399318 - Fix 2 one time leaks in nsKeygenHandler

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Kai Engert [:KaiE:]]

See bug 398665 comment 3.

This bug requests to fix that one time leak in nsKeygenHandler by converting the strings to be member variables (instead of global static pointers).

Comment 1

[Kai Engert [:KaiE:]]

Attachment: Patch v1

This patch was originally attached to bug 398665 comment 8.
I'm moving it over to this separate bug report.

Comment 2

[Robert Relyea]

Comment on attachment 284301 [details] [diff] [review]
Patch v1

OK, Init() is called on each new instance. The use of nsString in the class guarrentees that it will be freed when the the base object is freed.

If the fixed array style matches mozilla's fixed array declaration style, then it looks good to me.

Comment 3

[Kai Engert [:KaiE:]]

Comment on attachment 284301 [details] [diff] [review]
Patch v1

Hi David. While superreview is not strictly required for check in, I'll appreciate your review comments for this leak fix!

Comment 4

[Mike Schroepfer]

Comment on attachment 284301 [details] [diff] [review]
Patch v1

clearing the review request - if you don't want to wait for SR just re-nom

Comment 5

[Kai Engert [:KaiE:]]

Comment on attachment 284301 [details] [diff] [review]
Patch v1

If dbaron doesn't have time to sr this, we shouldn't wait for it.

Comment 6

[Kai Engert [:KaiE:]]

fixed

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 284301 [details] [diff] [review]
Patch v1

A few very late comments, since this request is still in my queue (not sure if you'll have a chance to address them):

> nsresult
> nsKeygenFormProcessor::Init()
> {
>   nsresult rv;
>-  nsAutoString str;
> 
>-  if (SECKeySizeChoiceList[0].name != NULL)
>-    return NS_OK;

I realize it's much less likely now, since it can't be done by a caller using do_CreateInstance instead of do_GetService, but it still might be worth asserting (NS_ASSERTION) that the string is empty with assertion text "Init called twice".

>+        if (aValue.Equals(mSECKeySizeChoiceList[i].name)) {

This could use "aValue == mSECKeySizeChoiceList[i].name" instead.

>+  enum { number_of_key_size_choices = 2 };
>+
>+  SECKeySizeChoiceInfo mSECKeySizeChoiceList[number_of_key_size_choices];

Rather than using an enum like this, it's probably simpler (and easier to verify correctness at the callsite) to use the NS_ARRAY_LENGTH macro, e.g.
;

for (size_t i = 0; i < NS_ARRAY_LENGTH(mSecKeySizeChoiceList); ++i) {
  // .. do something with mSecKeySizeChoiceList[i]
}


I also noticed  (unrelated to this patch) that nsKeygenFormProcessor::Create is a function probably better written using the NS_GENERIC_FACTORY_CONSTRUCTOR_INIT macro rather than being written out by hand.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #7)
> I also noticed  (unrelated to this patch) that nsKeygenFormProcessor::Create is
> a function probably better written using the
> NS_GENERIC_FACTORY_CONSTRUCTOR_INIT macro rather than being written out by
> hand.

... and that's something normally placed in the module file (nsNSSModule.cpp).

Comment 9

[Kai Engert [:KaiE:]]

(In reply to comment #7)
> A few very late comments, since this request is still in my queue (not sure if
> you'll have a chance to address them):
> 
> > nsresult
> > nsKeygenFormProcessor::Init()
> > {
> >   nsresult rv;
> >-  nsAutoString str;
> > 
> >-  if (SECKeySizeChoiceList[0].name != NULL)
> >-    return NS_OK;


Well, this code was removed!
Ok, a new assignment got added, however, there is no original zero-init in the constructor (yet).


> I realize it's much less likely now, since it can't be done by a caller using
> do_CreateInstance instead of do_GetService, but it still might be worth
> asserting (NS_ASSERTION) that the string is empty with assertion text "Init
> called twice".
> 
> >+        if (aValue.Equals(mSECKeySizeChoiceList[i].name)) {
> 
> This could use "aValue == mSECKeySizeChoiceList[i].name" instead.

But it's essentially equivalent.
I was not aware that == operators have now been implemented for our strings.

> 
> >+  enum { number_of_key_size_choices = 2 };
> >+
> >+  SECKeySizeChoiceInfo mSECKeySizeChoiceList[number_of_key_size_choices];
> 
> Rather than using an enum like this, it's probably simpler (and easier to
> verify correctness at the callsite) to use the NS_ARRAY_LENGTH macro, e.g.
> ;
> 
> for (size_t i = 0; i < NS_ARRAY_LENGTH(mSecKeySizeChoiceList); ++i) {
>   // .. do something with mSecKeySizeChoiceList[i]
> }


But we still need the enum to declare the size for the array in the header file?