Move PK11_HasAttributeSet to public header, doublecheck parameter list

RESOLVED FIXED in 3.30

Status

NSS
Libraries
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: kaie, Assigned: kaie)

Tracking

3.30
3.30
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

6.56 KB, patch
Robert Relyea
: review+
fkiefer
: review+
Details | Diff | Splinter Review
(Assignee)

Description

a year ago
In bug 1334976, we exported function PK11_HasAttributeSet.

I just noticed two details:

- the function is still located in a private header file, pk11priv.h

  We should probably move it to a public header, like p11pub.h


- the function signature includes a parameter, which might only be intended for 
  internal use: PRBool haslock

  I wonder if it's acceptable to export the function like that, or if we
  should change the function signature.

  Unfortunately Fedora has already started to use this function,
  in a backported patch. However, only in experimental/alpha branches.

  Should we rather change the code, to export a function without that parameter?
(Assignee)

Comment 1

a year ago
I think this bug should block the release, let's try to get it resolved quickly.
Assignee: nobody → kaie
Blocks: 1334127
(Assignee)

Comment 2

a year ago
Created attachment 8844417 [details] [diff] [review]
1345083-v1.patch
Attachment #8844417 - Flags: review?(rrelyea)

Updated

a year ago
Attachment #8844417 - Flags: review?(rrelyea) → review+

Comment 3

a year ago
OK, so ideally our public version should always have hasLock set to false. There's no way applications can already be holding the lock. If you pass haslock as true, then there is a chance that you get bad data.

I'm OK with a patch to export a different function that has a different name and drops the hasLock, but only if we are confident only non-released software is using this.

bob
(Assignee)

Comment 4

a year ago
(In reply to Robert Relyea from comment #3)
> ... but only if we are confident only non-released
> software is using this.

At least it would be tricky to clean this up in Fedora.

It's easier to keep the current public interface, and document that the hasLock parameter must always be set to false.
(Assignee)

Comment 5

a year ago
The exported API is part of the Fedora 26 alpha release.
(Assignee)

Comment 6

a year ago
Ok, let's avoid the hassle to revert an API that has already been released somewhere.

Let's use a patch to make things safe.

I've checked NSS, and there's only one place that calls PK11_HasAttributeSet with "hasLock != PR_FALSE".

I suggest the following patch, which:
- documents in the public API that the parameter must be set to PR_FALSE
- uses an assert to check the parameter is always false (in case I missed anything)
- changes the public API to a forwarding wrapper
- introduce a new private function, which uses the lock parameter
- only use the lock API at the one place where it's necessary

The call that has already been added to Firefox sets the parameter to false.
(Assignee)

Comment 7

a year ago
Created attachment 8844879 [details] [diff] [review]
1345083-v2.patch
(Assignee)

Updated

a year ago
Attachment #8844879 - Flags: review?(rrelyea)
(Assignee)

Comment 8

a year ago
Comment on attachment 8844879 [details] [diff] [review]
1345083-v2.patch

Optional review for Franziskus, if we're in a hurry to wrap up 3.30
Attachment #8844879 - Flags: review?(franziskuskiefer)
Comment on attachment 8844879 [details] [diff] [review]
1345083-v2.patch

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

::: lib/pk11wrap/pk11obj.c
@@ +182,5 @@
>  }
>  
> +CK_BBOOL
> +PK11_HasAttributeSet(PK11SlotInfo *slot, CK_OBJECT_HANDLE id,
> +                     CK_ATTRIBUTE_TYPE type, PRBool haslock)

How many uses of this are there? Couldn't we drop the haslock argument completely (it hasn't been in a release yet). We can easily change the use in Firefox. Not sure about the uses in Fedora (and others?).

@@ +184,5 @@
> +CK_BBOOL
> +PK11_HasAttributeSet(PK11SlotInfo *slot, CK_OBJECT_HANDLE id,
> +                     CK_ATTRIBUTE_TYPE type, PRBool haslock)
> +{
> +    PR_ASSERT(haslock == PR_FALSE);

If this is a strict requirement we should always check it not only in debug builds.
(Assignee)

Comment 10

a year ago
(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #9)
> >  
> > +CK_BBOOL
> > +PK11_HasAttributeSet(PK11SlotInfo *slot, CK_OBJECT_HANDLE id,
> > +                     CK_ATTRIBUTE_TYPE type, PRBool haslock)
> 
> How many uses of this are there? Couldn't we drop the haslock argument
> completely (it hasn't been in a release yet). We can easily change the use
> in Firefox. Not sure about the uses in Fedora (and others?).

It's already used by Fedora 26 alpha, which is already frozen, so it will be shipped.
The firefox in that alpha already calls that API.

It would require coordinated updates of firefox+nss after the f26 alpha. Doable in theory, but a hassle.


> @@ +184,5 @@
> > +CK_BBOOL
> > +PK11_HasAttributeSet(PK11SlotInfo *slot, CK_OBJECT_HANDLE id,
> > +                     CK_ATTRIBUTE_TYPE type, PRBool haslock)
> > +{
> > +    PR_ASSERT(haslock == PR_FALSE);
> 
> If this is a strict requirement we should always check it not only in debug
> builds.

The parameter is ignored, I just want to ensure that we don't have anyone setting the parameter accidentally.
Attachment #8844879 - Flags: review?(franziskuskiefer) → review+

Comment 11

a year ago
Comment on attachment 8844879 [details] [diff] [review]
1345083-v2.patch

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

This is a good patch. If we decide it's OK to drop the parameter, this patch is most of what we need for that as well. If we don't drop the parameter, we at least make the function safe.

bob

::: lib/pk11wrap/pk11obj.c
@@ +184,5 @@
> +CK_BBOOL
> +PK11_HasAttributeSet(PK11SlotInfo *slot, CK_OBJECT_HANDLE id,
> +                     CK_ATTRIBUTE_TYPE type, PRBool haslock)
> +{
> +    PR_ASSERT(haslock == PR_FALSE);

It's just a warning since we always pass false no matter what, so there's not down side to passing true, the assert just warns applications running with debug that the bool has no affect.
Attachment #8844879 - Flags: review?(rrelyea) → review+
(Assignee)

Comment 12

a year ago
Thanks for the reviews! I'm afraid we should indeed keep the API as is, to avoid trouble.

Landed on trunk:
https://hg.mozilla.org/projects/nss/rev/482e9cbb16f1

If green, I'll land into 3.30 branch shortly.
(Assignee)

Comment 13

a year ago
all green.

3.30 branch:
https://hg.mozilla.org/projects/nss/rev/af29a5e027de
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → 3.30
(Assignee)

Updated

a year ago
Depends on: 1334976
(Assignee)

Updated

a year ago
Attachment #8844417 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.