1318633 - Support initial init with noCertDB, later without noCertDB

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Kai Engert (:KaiE:)]

Attachment: database.patch

We found a regression, where an openldap tool uses NSS with the patch from bug 1312141, but didn't load the trust settings from the configured NSS database.

We found that when openldap (ldapsearch) initially initializes NSS, it includes the flag noCertDB.

But later on, it calls SECMOD_OpenUserDB, with the same database path, but no longer giving the noCertDB flag.

With the current state of NSS trunk, the second open will not open the database files, which is a regression from past behavior.

I'm attaching a patch that Bob has suggested, and which we have tested to fix the issue.

I'm asking Martin to review, because he was involved with bug 1312141.

Comment 1

[Martin Thomson [:mt:]]

Comment on attachment 8812139 [details] [diff] [review]
database.patch

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

I think that this is OK.

::: lib/pk11wrap/pk11pars.c
@@ +1014,5 @@
>      *certPrefix = NULL;
>      *keyPrefix = NULL;
>      *readOnly = NSSUTIL_ArgHasFlag("flags", "readOnly", spec);
> +    if (NSSUTIL_ArgHasFlag("flags","nocertdb", spec) ||
> +        NSSUTIL_ArgHasFlag("flags","nokeydb",spec)) {

Fix formatting please.

@@ +1136,5 @@
>                     char *certPrefix1, char *certPrefix2,
>                     char *keyPrefix1, char *keyPrefix2,
>                     PRBool isReadOnly1, PRBool isReadOnly2)
>  {
> +    if ((configDir1 == NULL) || (configDir2 == NULL)) {

Why not allow them to match if they are both NULL?  I realize that the cost there is that you don't reuse something without a database, which is probably trivial.  Maybe a comment to clarify why you think that this is OK.

Comment 2

[Max Liu [:maliu]]

Attachment: Bug 1318633 - Check launch intent action to determine skip new startup page tab or not,

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

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8814013 [details]
Bug 1318633 - Check launch intent action to determine skip new startup page tab or not,

I think you accidentally used the wrong bug number here :)

Comment 4

[Kai Engert (:KaiE:)]

Setting needinfo for Bob, because the patch was from Bob, so asking Bob to answer Martin's review questions/comments.

Comment 5

[Kai Engert (:KaiE:)]

I think this regression bug really needs to be fixed with the NSS 3.28 release.

It's a bit inconvenient that this couldn't get landed and completed earlier, but I think we need to delay the release, to allow this fix to get included.

Bob hasn't yet responded to the comments that Martin made 3 weeks ago. Nevertheless, Martin had given r+, and the only remaining task is a clarification comment from Bob.

So, I'd like to go ahead and check this in, together with a comment
  /* TODO: Add clarification why this code is correct */

Comment 6

[Kai Engert (:KaiE:)]

Checked in for NSS 3.29:
https://hg.mozilla.org/projects/nss/rev/8f8c3b2506a0

Comment 7

[Kai Engert (:KaiE:)]

Checked in for NSS 3.28:
https://hg.mozilla.org/projects/nss/rev/b8bbed415405

Although I'm marking this as fixed, it's still waiting for Bob to suggest a better comment, and needs a follow-up commit (although clarifying the comment on NSS trunk might be sufficient).

Comment 8

[Robert Relyea]

Sigh, That's weird, something swallowed up my response, probably Kai's comment 5? I know I made it (made a point to come here and post the comment).

Anyway as to martin's comments: The purpose of this function is to make sure we don't open the same database twice. NULL represents no database, so it's permissible to open no database twice.

I would be OK with the semantic that would return true in the both NULL case, but it's not really necessary, both NULL's is sort of a pathological case (probably a configuration error). I don't think we should crash, which is why I'm checking for NULL here.

bob