Closed Bug 1380706 Opened 2 years ago Closed 2 years ago

PSM should depend on mozStorage, as a workaround for a sqlite3_config race

Categories

(Core :: Security: PSM, enhancement, P1)

56 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox-esr52 --- ?
firefox56 --- fixed

People

(Reporter: KaiE, Assigned: KaiE)

References

Details

(Whiteboard: [psm-assigned])

Attachments

(1 file)

This suggests to implement a PSM-specific workaround for bug 730495, which is required for bug 783994.

(The workaround makes sense for now, because solving bug 730495 in general seems difficult and has unknown timing.)

As mentioned in bug 730495, the workaround is to require loading of mozStorage from within PSM module init, prior to calling NSS library init.

Once bug 730495 is fixed in a general way, this workaround could get removed.

The patch has already been reviewed in bug 730495 comment 34.

I'm attaching a second revision of the patch, which contains the changes that David had requested.
Attached patch 1380706-v2.patchSplinter Review
(In reply to David Keeler [:keeler] (use needinfo?) from bug 730495 comment #34)
> I think it might be best to put this in nsNSSComponent::Init so that it's
> impossible to instantiate PSM without the storage service. Also, since we're
> not checking rv, I think we can use the do_GetService variant that doesn't
> have the extra parameter.

New patch attached. Does the error code make sense?

try build:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6aa54693e2e6920cc8ee328085ce617494f3a3e7
Attachment #8886235 - Flags: review?(dkeeler)
Comment on attachment 8886235 [details] [diff] [review]
1380706-v2.patch

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

This looks good. Just the one nit and I think we should add a comment. Thanks!

::: security/manager/ssl/nsNSSComponent.cpp
@@ +56,5 @@
>  #include "sslerr.h"
>  #include "sslproto.h"
>  #include "prmem.h"
>  
> +#include "mozStorageCID.h"

nit: this should be earlier in the include list (it's intended to be sorted alphabetically, case-sensitive)

@@ +2038,5 @@
>  
> +  nsCOMPtr<nsISupports> storageService =
> +    do_GetService(MOZ_STORAGE_SERVICE_CONTRACTID);
> +  if (!storageService) {
> +    return NS_ERROR_NOT_AVAILABLE;

Yep - I think this is a good error code. Let's also have a comment explaining why we added this.
Attachment #8886235 - Flags: review?(dkeeler) → review+
Pushed by kaie@kuix.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8e722cacf61
PSM should depend on mozStorage, as a workaround for a sqlite3_config race, r=keeler
Thanks, pushed including the requested changes.
Priority: -- → P1
Whiteboard: [psm-assigned]
https://hg.mozilla.org/mozilla-central/rev/b8e722cacf61
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
No longer blocks: 730495
See Also: → 730495
for the record, this workaround was removed, as bug 730495 has been fixed.
Can you backport this to ESR52 to fix a crash --with-system-nss after bug 1377940?

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=225364
Flags: needinfo?(kaie)
Jan, several fixes were applied to Firefox to make it fully with the NSS SQL DB, this bug is just one of the related bugs. I don't think we can backport all of them. If you're using NSS 3.35+ as a shared system library, you could consider to patch NSS to keep the old DBM default, as long as you cannot yet upgrade to Firefox 58 or newer, and plan to accept the change of default in NSS at the same time when you will upgrade your environment to Firefox 58+.

In any way, this bug is probably not the right place for this discussion, I suggest the mozilla.dev.tech.crypto list.
Flags: needinfo?(kaie)
s/fully/fully compatible/
> several fixes were applied to Firefox to make it fully [compatible] with the NSS SQL DB, this bug is just one of the related bugs.

Is there a record of the applicable bugs for these "several fixes"? Backporting may be some work but for some projects hopping onto Quantum is simply a no-go area.
Mark, you can find the relevant work, by looking at bug 783994 and bug 1377940, and for each of them, by looking at their "depends on" lists.
Thanks Kal,

I've made note of it, if we have to revisit this later on (i.e. if we're forced to move to SQL which I hope won't be needed).

Our current solution is to prefix the cert db path with 'dbm:' to tell NSS we prefer DBM so we avoid this race condition and can keep using our preferred db format.
You need to log in before you can comment on or make changes to this bug.