InitCertVerifierLog is not called if PSM is initialized without a profile directory

RESOLVED FIXED in mozilla29

Status

()

Core
Security: PSM
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: keeler, Assigned: keeler)

Tracking

unspecified
mozilla29
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

5 years ago
InitCertVerifierLog is not called if PSM is initialized without a profile directory, which results in logging code dereferencing a null gCertVerifierLog.
(Assignee)

Comment 1

5 years ago
Created attachment 8367519 [details] [diff] [review]
patch
Assignee: nobody → dkeeler
Status: NEW → ASSIGNED
Attachment #8367519 - Flags: review?(brian)
Comment on attachment 8367519 [details] [diff] [review]
patch

Review of attachment 8367519 [details] [diff] [review]:
-----------------------------------------------------------------

Timely review is timely. The whole function is wrong.
Attachment #8367519 - Flags: review?(brian) → review-
(Assignee)

Updated

5 years ago
Attachment #8367519 - Attachment is obsolete: true
(Assignee)

Comment 3

5 years ago
Created attachment 8368275 [details] [diff] [review]
patch 1/2: fix indentation (diff -w version)
(Assignee)

Comment 4

5 years ago
Created attachment 8368276 [details] [diff] [review]
patch 1/2:fix indentation

This patch first fixes the indentation of nsNSSComponent::InitializeNSS() (see the diff -w version for an easier-to-review patch).
Attachment #8368276 - Flags: review?(brian)
(Assignee)

Comment 5

5 years ago
Created attachment 8368278 [details] [diff] [review]
patch 2/2: refactor getting profile directory

I refactored getting the profile directory and cleaned up the initialization a bit.
Attachment #8368278 - Flags: review?(brian)
Comment on attachment 8368278 [details] [diff] [review]
patch 2/2: refactor getting profile directory

Review of attachment 8368278 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +1051,5 @@
>  
>    return NS_OK;
>  }
>  
>  nsresult

static

@@ +1127,5 @@
> +  // this call for every re-init of NSS.
> +
> +  ConfigureInternalPKCS11Token();
> +
> +  InitCertVerifierLog();

IMO, it is better to put this call closer to the other CertVerifier initialization logic. The reason is that it will be more clear how the standalone embedded CertVerifier (bug 918629) initialization maps to the in-Gecko initialization.

@@ +1141,5 @@
>      SECStatus init_rv = NSS_NoDB_Init(nullptr);
>      if (init_rv != SECSuccess) {
>        nsPSMInitPanic::SetPanic();
>        return NS_ERROR_NOT_AVAILABLE;
>      }

NIt: should we try to coalesce this with the other cal to NSS_NoDB_Init that occurs after ::mozilla::psm::InitializeNSS fails?
Attachment #8368278 - Flags: review?(brian) → review+
Comment on attachment 8368275 [details] [diff] [review]
patch 1/2: fix indentation (diff -w version)

Review of attachment 8368275 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +1056,5 @@
>  nsNSSComponent::InitializeNSS()
>  {
>    // Can be called both during init and profile change.
>    // Needs mutex protection.
> +  MutexAutoLock lock(mutex);

I think this should go back to where it was originally, since neither the PR_LOG nor the static_assert require the lock.
Attachment #8368275 - Flags: review+
Comment on attachment 8368276 [details] [diff] [review]
patch 1/2:fix indentation

Review of attachment 8368276 [details] [diff] [review]:
-----------------------------------------------------------------

r+ assuming this is the same as the "diff -w" version + whitespace-only changes. See my comments on the other patch.
Attachment #8368276 - Flags: review?(brian) → review+
(Assignee)

Comment 9

5 years ago
Created attachment 8368325 [details] [diff] [review]
patch 2/2 v2: refactor getting profile directory

Thanks for the quick reviews, Brian. I changed where InitCertVerifierLog gets called and consolidated the calls to NSS_NoDB_Init.
Attachment #8368278 - Attachment is obsolete: true
Attachment #8368325 - Flags: review?(brian)
Comment on attachment 8368325 [details] [diff] [review]
patch 2/2 v2: refactor getting profile directory

Review of attachment 8368325 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +981,5 @@
>    CertVerifier::ocsp_get_config ogc;
>  
>    SetClassicOCSPBehaviorFromPrefs(&odc, &osc, &ogc, lock);
> +  // This is idempotent. Ensure the log module is initialized.
> +  InitCertVerifierLog();

Nit: should we initialize the log before SetClassicOCSPBehaviorFromPrefs in case SetClassicOCSPBehaviorFromPrefs ends up needing it at some point? i.e. Should we put it before the declaration of odc?

@@ +1153,5 @@
>    }
> +  // If we haven't succeeded in initializing the DB in our profile
> +  // directory or we don't have a profile at all, attempt to initialize
> +  // with no DB. Due to the setup above, this is equivalent to
> +  // if init_rv is not SECSuccess.

I suggest removing the last sentence, which has questionable grammar and is kind of stating the obvious.
Attachment #8368325 - Flags: review?(brian) → review+
(Assignee)

Comment 11

4 years ago
Created attachment 8369245 [details] [diff] [review]
patch 1/2 v2: fix indentation

Moved the mutex. Carrying over r+.
Attachment #8368275 - Attachment is obsolete: true
Attachment #8368276 - Attachment is obsolete: true
Attachment #8369245 - Flags: review+
(Assignee)

Comment 12

4 years ago
Created attachment 8369247 [details] [diff] [review]
patch 2/2 v3: refactor getting profile directory

Moved InitCertVerifierLog call to before any certverifier functions are called. Carring over r+.
https://tbpl.mozilla.org/?tree=Try&rev=0eb6f4cdeee5

https://hg.mozilla.org/integration/mozilla-inbound/rev/23639e7efe16
https://hg.mozilla.org/integration/mozilla-inbound/rev/68c546e8a848
Attachment #8368325 - Attachment is obsolete: true
Attachment #8369247 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/23639e7efe16
https://hg.mozilla.org/mozilla-central/rev/68c546e8a848
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.