FindObjects bool is always false about pkcs11

[dmjones]

This definitely looks broken to me.

It is impossible to determine whether C_FindObjects would have returned more values than max without calling it again. Only when C_FindObjects returns zero can you be sure the token has no more objects to return.

I think a breaking change is required to remove this boolean response value. It is misleading and I can't see how it can ever be accurate. It's already tripped me up on some code I wrote to work with SoftHSM2.

In a pathological case, you cannot even be sure the token has run out of objects if ulCount < maz. The token is under no obligation to always fill the available space. Its only obligation is to return 0 if no more objects exist.

The workaround for now is to ignore this boolean value and keep calling until no objects are returned.

[dmjones]

@miekg Can we reopen this?

[mtharp]

Your interpretation of the spec seems correct to me. It's unclear whether changing the code would be a good idea since implementations may be depending on it never returning true (see this example in the pkcs11 tree: https://github.com/miekg/pkcs11/blob/master/p11/session.go#L75)

The safest fix would be updating the comment to mark it as deprecated and describing what the correct use should be.

[miekg]

[ Quoting <[email protected]> in "Re: [miekg/pkcs11] FindObjects bool..." ]

Your interpretation of the spec seems correct to me. It's unclear whether changing the code would be a good idea since implementations may be depending on it never returning true (see this example *in the pkcs11 tree*: https://github.com/miekg/pkcs11/blob/master/p11/session.go#L75) The safest fix would be updating the comment to mark it as deprecated and describing what the correct use should be.

A backwards incompatible change could be fine if we break builds. Do we need another function that is correct, while deprecating this one?

[mtharp]

The existing signature is sufficient to use the function correctly, you just ignore the boolean and stop searching when the result is empty. So there's not really any benefit to changing it until there are other breaking changes to bundle it with. I opened a pull request to update the docs and fix the p11 version of the function to not use it.

[dmjones]

I'd opt for a breaking change. Removing a return value will break builds, so people won't miss this happening. People who are ignoring the return value are minimally inconvenienced. People with broken code that assumes it works correctly will have more work to do, but they had buggy code anyway.

I fear people will miss documentation changes.

[miekg]

[ Quoting <[email protected]> in "Re: [miekg/pkcs11] FindObjects bool..." ]

I'd opt for a breaking change. Removing a return value will break builds, so people won't miss this happening. People who are ignoring the return value are minimally inconvenienced. People with broken code that assumes it works correctly will have more work to do, but they had buggy code anyway. I fear people *will* miss documentation changes.

They will indeed. "dep ensure" or something similar will not tell you, just breaking build seems legit here.

[mtharp]

Should #55 be addressed at the same time then?

Ideally there should be a tag before the breaking change so that people using dependency management can pin it until they can update their code.

[miekg]

If we're going to break things, its better to group those, so yes?

releases is a separate issues: now with vgo we should actually start doing this.