1345083 - Move PK11_HasAttributeSet to public header, doublecheck parameter list

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement)

Description

[Kai Engert (:KaiE:)]

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?

Comment 1

[Kai Engert (:KaiE:)]

I think this bug should block the release, let's try to get it resolved quickly.

Comment 2

[Kai Engert (:KaiE:)]

Attachment: 1345083-v1.patch

Comment 3

[Robert Relyea]

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

Comment 4

[Kai Engert (:KaiE:)]

(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.

Comment 5

[Kai Engert (:KaiE:)]

The exported API is part of the Fedora 26 alpha release.

Comment 6

[Kai Engert (:KaiE:)]

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.

Comment 7

[Kai Engert (:KaiE:)]

Attachment: 1345083-v2.patch

Comment 8

[Kai Engert (:KaiE:)]

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

Comment 9

[Franziskus Kiefer [:franziskus]]

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.

Comment 10

[Kai Engert (:KaiE:)]

(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.

Comment 11

[Robert Relyea]

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.

Comment 12

[Kai Engert (:KaiE:)]

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.

Comment 13

[Kai Engert (:KaiE:)]

all green.

3.30 branch:
https://hg.mozilla.org/projects/nss/rev/af29a5e027de