Closed Bug 1318633 Opened 3 years ago Closed 3 years ago

Support initial init with noCertDB, later without noCertDB

Categories

(NSS :: Libraries, defect)

3.28
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: kaie, Assigned: rrelyea)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

Attached patch database.patchSplinter Review
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.
Attachment #8812139 - Flags: review?(martin.thomson)
Attachment #8812139 - Flags: feedback+
Blocks: 1305970
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.
Attachment #8812139 - Flags: review?(martin.thomson) → review+
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 :)
Attachment #8814013 - Attachment is obsolete: true
Attachment #8814013 - Flags: review?(s.kaspari)
Setting needinfo for Bob, because the patch was from Bob, so asking Bob to answer Martin's review questions/comments.
Flags: needinfo?(rrelyea)
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 */
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).
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
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
Flags: needinfo?(rrelyea)
You need to log in before you can comment on or make changes to this bug.