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)
NSS
Libraries
Tracking
(Not tracked)
RESOLVED
FIXED
3.39
People
(Reporter: dcb314, Assigned: elio.maldonado.batiz)
Details
(Keywords: good-first-bug)
Attachments
(2 files, 2 obsolete files)
1.28 KB,
patch
|
Details | Diff | Splinter Review | |
685 bytes,
patch
|
franziskus
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•7 years ago
|
Assignee: nobody → nobody
Component: Untriaged → Libraries
Product: Firefox → NSS
Version: 54 Branch → other
Comment 1•7 years ago
|
||
Nice spot. We should fix the declaration in ckfw.h.
Comment 2•7 years ago
|
||
How to work out which of the arguments is read-only and which is written to?
Attachment #8995234 -
Flags: review?(elio.maldonado.batiz)
Comment 3•7 years ago
|
||
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?
Assignee | ||
Comment 4•7 years ago
|
||
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);
Assignee | ||
Updated•7 years ago
|
Attachment #8995234 -
Flags: review?(elio.maldonado.batiz) → review+
Assignee | ||
Comment 5•7 years ago
|
||
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,
Assignee | ||
Comment 6•7 years ago
|
||
Attachment #8995234 -
Attachment is obsolete: true
Attachment #8997947 -
Flags: review?(franziskuskiefer)
Assignee | ||
Updated•7 years ago
|
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
Assignee | ||
Comment 7•7 years ago
|
||
Updated•7 years ago
|
Attachment #8997947 -
Flags: review?(franziskuskiefer) → review+
Assignee | ||
Comment 8•7 years ago
|
||
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.
Assignee | ||
Comment 9•7 years ago
|
||
Patch expanded per Comment 8.
Attachment #8997947 -
Attachment is obsolete: true
Assignee | ||
Comment 10•7 years ago
|
||
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Comment 11•7 years ago
|
||
The call-sites need updating to cast away const if necessary.
Flags: needinfo?(elio.maldonado.batiz)
Assignee | ||
Comment 12•7 years ago
|
||
(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)
Assignee | ||
Comment 13•7 years ago
|
||
Fixes the problem you pointed out.
Attachment #8998252 -
Flags: review?(franziskuskiefer)
Comment 14•7 years ago
|
||
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+
Assignee | ||
Comment 15•7 years ago
|
||
Comment 16•7 years ago
|
||
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)
Updated•7 years ago
|
Target Milestone: --- → 3.39
Assignee | ||
Comment 17•7 years ago
|
||
(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)
Assignee | ||
Comment 18•7 years ago
|
||
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)
Comment 19•7 years ago
|
||
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)
Comment 20•7 years ago
|
||
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.
Updated•7 years ago
|
Assignee: nobody → elio.maldonado.batiz
You need to log in
before you can comment on or make changes to this bug.
Description
•