Closed Bug 1394098 Opened 7 years ago Closed 7 years ago

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

Categories

(NSS :: Libraries, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: dcb314, Assigned: elio.maldonado.batiz)

Details

(Keywords: good-first-bug)

Attachments

(2 files, 2 obsolete files)

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.
Assignee: nobody → nobody
Component: Untriaged → Libraries
Product: Firefox → NSS
Version: 54 Branch → other
Nice spot. We should fix the declaration in ckfw.h.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: good-first-bug
Priority: -- → P3
Attached patch bug-1394908-01.patch (obsolete) — Splinter Review
How to work out which of the arguments is read-only and which is written to?
Attachment #8995234 - Flags: review?(elio.maldonado.batiz)
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?
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);
Attachment #8995234 - Flags: review?(elio.maldonado.batiz) → review+
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,
Attachment #8995234 - Attachment is obsolete: true
Attachment #8997947 - Flags: review?(franziskuskiefer)
Summary: firefox-55.0.3/security/nss/lib/ckfw/ckfw.h:1606: bad declaration ? → firefox-55.0.3/security/nss/lib/ckfw/ckfw.h:1606: bad declaration
Attachment #8997947 - Flags: review?(franziskuskiefer) → review+
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.
Patch expanded per Comment 8.
Attachment #8997947 - Attachment is obsolete: true
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
The call-sites need updating to cast away const if necessary.
Flags: needinfo?(elio.maldonado.batiz)
(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.
Flags: needinfo?(elio.maldonado.batiz)
Fixes the problem you pointed out.
Attachment #8998252 - Flags: review?(franziskuskiefer)
Comment on attachment 8998252 [details] [diff] [review] cast away const qualifier on SetPin call Review of attachment 8998252 [details] [diff] [review]: ----------------------------------------------------------------- thanks!
Attachment #8998252 - Flags: review?(franziskuskiefer) → review+
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.
Flags: needinfo?(elio.maldonado.batiz)
Target Milestone: --- → 3.39
(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.
Flags: needinfo?(elio.maldonado.batiz)
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.
Flags: needinfo?(rrelyea)
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
Flags: needinfo?(rrelyea)
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.
Assignee: nobody → elio.maldonado.batiz
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: