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

RESOLVED FIXED in 3.39

Status

P3
normal
RESOLVED FIXED
a year ago
a month ago

People

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

Tracking

({good-first-bug})

other
3.39
good-first-bug

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

a year ago
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

Comment 2

2 months ago
Created attachment 8995234 [details] [diff] [review]
bug-1394908-01.patch

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?
(Assignee)

Comment 4

2 months 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

2 months ago
Attachment #8995234 - Flags: review?(elio.maldonado.batiz) → review+
(Assignee)

Comment 5

2 months 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

2 months ago
Created attachment 8997947 [details] [diff] [review]
Ventakesh's patch with the suggested modification
Attachment #8995234 - Attachment is obsolete: true
Attachment #8997947 - Flags: review?(franziskuskiefer)
(Assignee)

Updated

2 months 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
Attachment #8997947 - Flags: review?(franziskuskiefer) → review+
(Assignee)

Comment 8

a month 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

a month ago
Created attachment 8998221 [details] [diff] [review]
Ventakesh's patch with the suggested modifications

Patch expanded per Comment 8.
Attachment #8997947 - Attachment is obsolete: true
(Assignee)

Comment 10

a month ago
Pushed: https://hg.mozilla.org/projects/nss/rev/f89a359d962a875c8729ce0a4a750566f486a9f0
Status: NEW → RESOLVED
Last Resolved: a month ago
Resolution: --- → FIXED
The call-sites need updating to cast away const if necessary.
Flags: needinfo?(elio.maldonado.batiz)
(Assignee)

Comment 12

a month 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

a month ago
Created attachment 8998252 [details] [diff] [review]
cast away const qualifier on SetPin call

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)

Updated

a month ago
Target Milestone: --- → 3.39
(Assignee)

Comment 17

a month 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

a month 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

a month 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)
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

a month ago
Assignee: nobody → elio.maldonado.batiz
You need to log in before you can comment on or make changes to this bug.