1394098 - firefox-55.0.3/security/nss/lib/ckfw/ckfw.h:1606: bad declaration

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P3)

Description

[David Binderman]

User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID: 20170613080647

Steps to reproduce:

firefox-55.0.3/security/nss/lib/ckfw/ckfw.h:1606] -> [firefox-55.0.3/security/nss/lib/ckfw/session.c:873]: (warning) Function 'nssCKFWSession_SetPIN' argument order different: declaration 'fwSession, newPin, oldPin' definition 'fwSession, oldPin, newPin'

In the header file:

NSS_EXTERN CK_RV
nssCKFWSession_SetPIN(
    NSSCKFWSession *fwSession,
    NSSItem *newPin,
    NSSItem *oldPin);

In the C file

NSS_IMPLEMENT CK_RV
nssCKFWSession_SetPIN(
    NSSCKFWSession *fwSession,
    NSSItem *oldPin,
    NSSItem *newPin)

Suggest change the header file and make sure all callers of this function have
the parameters the correct way around.

Also suggest mark one of the parameters const to make clear which one
is being read from and which one is being written to.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Nice spot. We should fix the declaration in ckfw.h.

Comment 2

[Venkatesh (:vpit3833)]

Attachment: bug-1394908-01.patch

How to work out which of the arguments is read-only and which is written to?

Comment 3

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8995234 [details] [diff] [review]
bug-1394908-01.patch

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

Did you make sure that all callers of this function have the parameters the correct way around?

Comment 4

[Elio Maldonado]

Not too many callers:
[emaldonado@fedora-26 code]$ grep -r nssCKFWSession_SetPIN nss
nss/lib/ckfw/session.c: *  nssCKFWSession_SetPIN
nss/lib/ckfw/session.c: * nssCKFWSession_SetPIN
nss/lib/ckfw/session.c:nssCKFWSession_SetPIN(
nss/lib/ckfw/ckfw.h: *  nssCKFWSession_SetPIN
nss/lib/ckfw/ckfw.h: * nssCKFWSession_SetPIN
nss/lib/ckfw/ckfw.h:nssCKFWSession_SetPIN(
nss/lib/ckfw/wrap.c:    error = nssCKFWSession_SetPIN(fwSession, oldArg, newArg);

Comment 5

[Elio Maldonado]

Comment on attachment 8995234 [details] [diff] [review]
bug-1394908-01.patch

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

::: lib/ckfw/ckfw.h
@@ +1604,4 @@
>  NSS_EXTERN CK_RV
>  nssCKFWSession_SetPIN(
>      NSSCKFWSession *fwSession,
> +    NSSItem *oldPin,

I forgot this suggestion 
"Also suggest mark one of the parameters const to make clear which one is being read from and which one is being written to."
I agree, should be
 const NSSItem *oldPin,

Comment 6

[Elio Maldonado]

Attachment: Ventakesh's patch with the suggested modification

Comment 7

[Elio Maldonado]

pushed to nss-try: https://hg.mozilla.org/projects/nss-try/rev/3da026fc66e7ceaa227014d5a997fd04693ba776

Comment 8

[Elio Maldonado]

I realized that if we add the "const" qualifier in the ckfw.h header we must also do it in the session.c implementation file.

Comment 9

[Elio Maldonado]

Attachment: Ventakesh's patch with the suggested modifications

Patch expanded per Comment 8.

Comment 10

[Elio Maldonado]

Pushed: https://hg.mozilla.org/projects/nss/rev/f89a359d962a875c8729ce0a4a750566f486a9f0

Comment 11

[Franziskus Kiefer [:franziskus]]

The call-sites need updating to cast away const if necessary.

Comment 12

[Elio Maldonado]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #11)
> The call-sites need updating to cast away const if necessary.

Thanks for pointing that out on tree-herder, 
"../../lib/ckfw/session.c:872:1: error: conflicting types for ‘nssCKFWSession_SetPIN’
Patch coming for review.

Comment 13

[Elio Maldonado]

Attachment: cast away const qualifier on SetPin call

Fixes the problem you pointed out.

Comment 14

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8998252 [details] [diff] [review]
cast away const qualifier on SetPin call

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

thanks!

Comment 15

[Elio Maldonado]

Pushed: https://hg.mozilla.org/projects/nss/rev/187c81f830117f7a58c7f843804458c9b0c4452e

Comment 16

[Martin Thomson [:mt:]]

Elio, I see your bustage fix used a cast.  Can you instead change the type of oldArg in NSSCKFWC_SetPIN() https://searchfox.org/nss/rev/397d9b45e399c0035d510d7af48b1aec38a6ec05/lib/ckfw/wrap.c#1148 to be const and the argument to be const?  Let's not make our const-correctness worse unless we have to.

Comment 17

[Elio Maldonado]

(In reply to Martin Thomson [:mt:] from comment #16)
> Elio, I see your bustage fix used a cast.  Can you instead change the type
> of oldArg in NSSCKFWC_SetPIN()
> https://searchfox.org/nss/rev/397d9b45e399c0035d510d7af48b1aec38a6ec05/lib/
> ckfw/wrap.c#1148 to be const and the argument to be const?  Let's not make
> our const-correctness worse unless we have to.

You are absolutely right, patch coming next.

Comment 18

[Elio Maldonado]

Working on this candidate patch https://hg.mozilla.org/projects/nss-try/rev/beff10fbff8e99872794c39ae918a25b9baf64c2
but unfortunately the try build https://treeherder.mozilla.org/#/jobs?repo=nss-try&selectedJob=192871560 
shows me that the build bustage is back, error message is:
../../lib/ckfw/session.c:910:65: error: passing argument 7 of ‘fwSession->mdSession->SetPIN’ discards ‘const’ qualifier from pointer target type [-Werror=discarded-qualifiers]

Searching through the code I also found references to the call in ck.api and pkcs11f.h.
Look at https://dxr.mozilla.org/nss/source/nss/lib/ckfw/ck.api#84 and following lines.
Also https://dxr.mozilla.org/nss/source/nss/lib/ckfw/ck.api#6 states taht the file is derived from pkcs11f.h
https://dxr.mozilla.org/nss/source/nss/lib/util/pkcs11f.h - a public PKCS #11 interface that I suspect is off-limits
for me to be modifying. Don't yet how to deal with this problem without touching it. Let's see what Bob has to say.

Comment 19

[Robert Relyea]

Yes, util/pkcs11f.h is an api definition set by the PKCS #11 standard. I think const is missing from most of the PKCS #11 standard. You'll probably just have to cast it for the callers.

Fortunately it shouldn't affect ckfw since ckfw sits below pkcs11, and implicit cast from non-const to const is allowed. 
The code you are looking at is the ckfw implementation of pkcs11f.h. It should have the non-const interface described by pkcs11f.h, but should pass the non-const to the const internal functions.

bob

Comment 20

[Martin Thomson [:mt:]]

So Elio, your hack was ideal.  Let's not worry about trying to fix pkcs11f.h.  The PKCS#11 folks can consider how much they care about const-correctness at their leisure.  I'm sure it's not that high a priority.