160122 - Stop using PR_smprintf in PSM

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Kai Engert [:KaiE:]]

Avoid extra allocations, replace PR_smprintf with nsCAutoString

Change requested by alecf. See bug 74339 comment 20.

Comment 1

[Stephane Saux]

kaie
Is this something you need to do now?

Comment 2

[Kai Engert [:KaiE:]]

changing obsolete psm* target to --- (unspecified)

Comment 3

[:Cykesiopka]

Actually, let's go a step further and stop using PR_smprintf entirely.

We can use the (more) modern Mozilla string classes instead, which at the very least provide built in automatic memory management and performance improvements.

Comment 4

[:Cykesiopka]

Attachment: MozReview Request: Bug 160122 - Stop using PR_smprintf in PSM. r=keeler

PSM can use the (more) modern Mozilla string classes instead, which at the very
least provide built in automatic memory management and performance improvements.

Review commit: https://reviewboard.mozilla.org/r/51659/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51659/

Comment 5

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8750828 [details]
MozReview Request: Bug 160122 - Stop using PR_smprintf in PSM. r=keeler

https://reviewboard.mozilla.org/r/51659/#review48467

LGTM.

::: security/certverifier/NSSCertDBTrustDomain.cpp:1009
(Diff revision 1)
>      fullLibraryPath(PR_GetLibraryName(dir, "nssckbi"), PR_FreeLibraryName);
>    if (!fullLibraryPath) {
>      return SECFailure;
>    }
>  
>    UniquePORTString escapedFullLibraryPath(nss_addEscape(fullLibraryPath.get(),

This should be a follow-up, but another candidate for modernization would be nss_addEscape.

::: security/certverifier/NSSCertDBTrustDomain.cpp:1027
(Diff revision 1)
> -  if (!pkcs11ModuleSpec) {
>      return SECFailure;
>    }
>  
> -  UniqueSECMODModule rootsModule(SECMOD_LoadUserModule(pkcs11ModuleSpec.get(),
> -                                                       nullptr, false));
> +  UniqueSECMODModule rootsModule(
> +    SECMOD_LoadUserModule(const_cast<char*>(pkcs11ModuleSpec.get()), nullptr,

I think we can avoid the const_cast if we use GetMutableData, but since this is really a result of a poorly stated API rather than that we're actually modifying the data, I'm not sure it's worth it.

::: security/certverifier/NSSCertDBTrustDomain.cpp:1079
(Diff revision 1)
> -    }
> +  }
> +  if (!baseName) {
> +    return NS_ERROR_FAILURE;
>    }
>  
> -  count = 1;
> +  // XXX: Give up after a certain number of tries?

Yeah, we can probably give up after 500 or so here. Looks like this is only used in caching encountered intermediate certificates and keeping a copy of server certificates corresponding to certificate error overrides. If either fail, it's not a big deal (the overall task will still succeed). If they take forever, it's more of a problem.

::: security/certverifier/NSSCertDBTrustDomain.cpp:1131
(Diff revision 1)
>      // We have found a signer cert that we want to remember.
> -    char* nickname = DefaultServerNicknameForCert(node->cert);
> -    if (nickname && *nickname) {
> +    nsAutoCString nickname;
> +    nsresult rv = DefaultServerNicknameForCert(node->cert, nickname);
> +    if (NS_SUCCEEDED(rv)) {
>        UniquePK11SlotInfo slot(PK11_GetInternalKeySlot());
>        if (slot) {

If I recall correctly, other areas of code generally treat PK11_GetInternalKeySlot failure as fatal - it might be good to be consistent.

Comment 6

[:Cykesiopka]

https://reviewboard.mozilla.org/r/51659/#review48467

Thanks!

> This should be a follow-up, but another candidate for modernization would be nss_addEscape.

I filed Bug 1271953.

> I think we can avoid the const_cast if we use GetMutableData, but since this is really a result of a poorly stated API rather than that we're actually modifying the data, I'm not sure it's worth it.

Yeah, I think using const_cast here is slightly better - we don't actually want the string to be modified, so GetMutableData would be misleading.

> Yeah, we can probably give up after 500 or so here. Looks like this is only used in caching encountered intermediate certificates and keeping a copy of server certificates corresponding to certificate error overrides. If either fail, it's not a big deal (the overall task will still succeed). If they take forever, it's more of a problem.

Sounds good.

> If I recall correctly, other areas of code generally treat PK11_GetInternalKeySlot failure as fatal - it might be good to be consistent.

Yes, I believe you're correct - I'll make the error fatal.

While I'm here, I'll also move the PK11_GetInternalKeySlot() call outside the loop since it doesn't seem to need to be inside the loop, and I'll add a comment about why it's OK to ignore the return value of PK11_ImportCert().

Comment 7

[:Cykesiopka]

Comment on attachment 8750828 [details]
MozReview Request: Bug 160122 - Stop using PR_smprintf in PSM. r=keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51659/diff/1-2/

Comment 8

[:Cykesiopka]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=759e0239b0d2

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0eeddfe247f7

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0eeddfe247f7