552951 - nsKeygenFormProcessor::GetPublicKey doesn't use pqgParams/arena created by decode_pqg_params

Status: RESOLVED FIXED

(Core Graveyard :: Security: UI, defect)

Description

[timeless]

505 nsKeygenFormProcessor::GetPublicKey(nsAString& aValue, nsAString& 
516     PQGParams *pqgParams;

579 found_match:
580         pqgParams = decode_pqg_params(str);

Comment 1

[timeless]

Attachment: patch v1

kaie: 

@@ -577,7 +576,7 @@ nsKeygenFormProcessor::GetPublicKey(nsAS

is the real change.

does this really make sense?

Comment 2

[Kai Engert [:KaiE:]]

The interesting piece from your patch, you found that the value created in the following line is never used.


diff --git a/security/manager/ssl/src/nsKeygenHandler.cpp b/security/manager/ssl/src/nsKeygenHandler.cpp
--- a/security/manager/ssl/src/nsKeygenHandler.cpp
+++ b/security/manager/ssl/src/nsKeygenHandler.cpp
@@ -577,7 +576,7 @@ nsKeygenFormProcessor::GetPublicKey(nsAS
         } while (end != nsnull);
         goto loser;
 found_match:
         pqgParams = decode_pqg_params(str);     <-------


According to CVS blame, this hasn't changed since initial CVS checkin.

In function nsKeygenFormProcessor::GetPublicKey, variable pqgParams was never used.

Comment 3

[Kai Engert [:KaiE:]]

Comment on attachment 495386 [details] [diff] [review]
patch v1

I believe the call to decode_pqg_params was accidentally left in the code. I'm guessing the original code was doing things inline, and now this check is being done inside function pqg_prime_bits.

The call to decode_pqg_params doesn't have any effects besides creating the leaked return object.

I agree it's OK to remove this call and the variable.

I'm also OK with your other cleanup in this patch.

I'm not happy about introducing
  (void)0;
I understand, though, it's necessary because of the goto label. Sigh.

Therefore, I give r=kaie for this patch, but would like to see a follow up patch that removes this ugliness and gets rid of the goto.
I'll attach that follow up patch.

Comment 4

[Kai Engert [:KaiE:]]

Attachment: incremental patch v2

Unrelated to the bug, patch on top of v1.
Replacing "goto found_match" and ugly statements with a bool.

Comment 5

[Honza Bambas (:mayhemer)]

Comment on attachment 499367 [details] [diff] [review]
incremental patch v2

r=honzab

Comment 6

[Kai Engert [:KaiE:]]

Comment on attachment 499367 [details] [diff] [review]
incremental patch v2

Note to drivers:

Please approve both patches.

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 495386 [details] [diff] [review]
patch v1

At this point drivers need to know why you want this and what the risk is. Feel free to renominate with those.

Comment 8

[Kai Engert [:KaiE:]]

The keywords say it all: We want it, because we want Firefox to be more correct and have less memory leaks.

What flag am I supposed to set, so we don't forget to land this later on?

Comment 9

[Honza Bambas (:mayhemer)]

Probably make it dep on bug 610267.

Comment 10

[Kai Engert [:KaiE:]]

(In reply to comment #9)
> Probably make it dep on bug 610267.

Thanks a lot, that helps!

Comment 11

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/97315604c128

Comment 12

[(no longer active)]

Also, http://hg.mozilla.org/mozilla-central/rev/b2c9d7ea0d38

Comment 13

[u279076]

Can someone confirm if this is fixed?