1223316 - [CID 1242519] Position of arguments to nssCKFWSession_SetPIN don't match

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Franziskus Kiefer [:franziskus]]

In wrap.c we call 
> nssCKFWSession_SetPIN(fwSession, oldArg, newArg); 
in NSSCKFWC_SetPIN. But nssCKFWSession_SetPIN is defined as
> nssCKFWSession_SetPIN(NSSCKFWSession *fwSession, NSSItem *newPin, NSSItem *oldPin)

This looks wrong to Coverity (and me).

Comment 1

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

It looks like they're reversed again when calling the SetPIN method; the type is:

  CK_RV (PR_CALLBACK *SetPIN)(
    // (first 6 args removed for clarity)
    NSSItem *oldPin,
    NSSItem *newPin
  );

But the call in nssCKFWSession_SetPIN is:

  error = fwSession->mdSession->SetPIN(/* ... */, newPin, oldPin);

Comment 2

[Franziskus Kiefer [:franziskus]]

right, so it works, but the variable names make no sense :) I think we should rename the arguments in nssCKFWSession_SetPIN then to (NSSCKFWSession *fwSession, NSSItem *oldPin, NSSItem *newPin).

Comment 3

[Franziskus Kiefer [:franziskus]]

Attachment: renaming arguments

This is not actually a bug, but we should use proper variable names here to not confuse people (and coverity).

Comment 4

[Martin Thomson [:mt:]]

Comment on attachment 8685855 [details] [diff] [review]
renaming arguments

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

I checked the functions either side of this and the old/new arguments are the other way around.  This seems to be correct.

Comment 5

[Martin Thomson [:mt:]]

https://hg.mozilla.org/projects/nss/rev/4c2e358644e1