Closed Bug 1250256 Opened 4 years ago Closed 4 years ago

Partially clean up nsSDR.cpp

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: Cykesiopka, Assigned: Cykesiopka)

References

Details

Attachments

(1 file)

nsSDR.cpp has some issues we can fix:
 - Use of deprecated Scoped.h features
 - gotos
 - Variables declared far from where they are actually used
 - Unnecessary variables

There are other issues like manual memory management and unimplemented methods, but those can/will be fixed later.
https://reviewboard.mozilla.org/r/37015/#review33607

::: security/manager/ssl/tests/unit/test_sdr.js:25
(Diff revision 1)
> +  // We have to login with the default password before we attempt to encrypt
> +  // or decrypt anything.

It's more useful to actually say why we need to do this.
Comment on attachment 8724364 [details]
MozReview Request: Bug 1250256 - Partially clean up nsSDR.cpp. r=keeler

https://reviewboard.mozilla.org/r/37015/#review34021

Looks good - just a few comments.

::: security/manager/ssl/nsSDR.cpp:145
(Diff revision 1)
>  EncryptString(const char *text, char **_retval)

This function could use some cleanup as well (e.g. * alignment, NS_IMETHODIMP on its own line, nsSecretDecoderRing::EncryptString all together, no need for the shutdown lock here, use of goto, manually-managed memory, etc.)
Oh, I see - the intention wasn't to clean everything up. Well, it's up to you - this feels like a fairly easy win, but it's been this way for a while, so I can't imagine it would hurt to leave it some more.

::: security/manager/ssl/nsSDR.cpp:169
(Diff revision 1)
>  DecryptString(const char *crypt, char **_retval)

Same here.

::: security/manager/ssl/nsSDR.cpp:246
(Diff revision 1)
>      nsNSSShutDownPreventionLock locker;

This needs if (isAlreadyShutDown()) { return NS_ERROR_NOT_AVAILABLE; }

::: security/manager/ssl/nsSDR.cpp:265
(Diff revision 1)
>      nsNSSShutDownPreventionLock locker;

Same here.

::: security/manager/ssl/tests/unit/head_psm.js:14
(Diff revision 1)
> -var { ctypes } = Cu.import("resource://gre/modules/ctypes.jsm");
> +const { MockRegistrar } =

Factoring out XPCOMUtils seems fine, but let's not add new things that only one test file needs (unless there's a reason they need to be loaded in head_psm.js?)

::: security/manager/ssl/tests/unit/test_sdr.js:54
(Diff revision 1)
> +    equal(input, sdr.decryptString(encrypted),

Is the SDR deterministic? I think the default key is just "" - it might be worth it to compare encrypted to the expected encrypted value, if it is the same every time.
Attachment #8724364 - Flags: review?(dkeeler) → review+
https://reviewboard.mozilla.org/r/37015/#review34021

Thanks for the review!

> This function could use some cleanup as well (e.g. * alignment, NS_IMETHODIMP on its own line, nsSecretDecoderRing::EncryptString all together, no need for the shutdown lock here, use of goto, manually-managed memory, etc.)
> Oh, I see - the intention wasn't to clean everything up. Well, it's up to you - this feels like a fairly easy win, but it's been this way for a while, so I can't imagine it would hurt to leave it some more.

I made some of the additional changes.

I have a long term goal of eradicating goto and manually managed memory in PSM, so hopefully I will get the remaining stuff cleaned up at some point.

> Factoring out XPCOMUtils seems fine, but let's not add new things that only one test file needs (unless there's a reason they need to be loaded in head_psm.js?)

Yeah, I was just being lazy, since a test file I'm adding in Bug 1250258 will make use of MockRegistrar as well.
I plan on submitting the patch for that within a week or two, so I think keeping MockRegistrar here is OK for now.

> Is the SDR deterministic? I think the default key is just "" - it might be worth it to compare encrypted to the expected encrypted value, if it is the same every time.

Looks like at least in the context in which we use the SDR here, no. The encrypted value for the same input changes each time the test file is run.
Comment on attachment 8724364 [details]
MozReview Request: Bug 1250256 - Partially clean up nsSDR.cpp. r=keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37015/diff/1-2/
Attachment #8724364 - Attachment description: MozReview Request: Bug 1250256 - Partially clean up nsSDR.cpp. → MozReview Request: Bug 1250256 - Partially clean up nsSDR.cpp. r=keeler
https://hg.mozilla.org/mozilla-central/rev/a19efafe26ac
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Blocks: 1260642
You need to log in before you can comment on or make changes to this bug.