GetFingerprint in PeerConnectionImpl.cpp is broken

RESOLVED FIXED in mozilla28

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: ryan, Assigned: mt)

Tracking

Trunk
mozilla28
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 1 obsolete attachment)

Currently PeerConnectionImpl::GetFingerprint in PeerConnectionImpl.cpp is commented out, and uncommenting won't build.
Attachment #819865 - Flags: review?(adam)
Blocks: 884573
Comment on attachment 819865 [details] [diff] [review]
0001-Make-DTLS-fingerprint-accessible-from-JS.patch

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

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +752,4 @@
>    // DTLS identity
>    unsigned char fingerprint[DTLS_FINGERPRINT_LENGTH];
>    size_t fingerprint_length;
> +  res = mIdentity->ComputeFingerprint("sha-256",

I always get a little worried when you have code that looks like this:
x = thing->doSomething(...);
y = thing->doNextThing(x, ...);
z = thing->keepGoing(y, ...);

PeerConnectionImpl.cpp shouldn't know that sha-256 is the thing to use.  That's an identity thing.  Have the identity class deal with all that stuff.
Attachment #819865 - Flags: review?(martin.thomson)
Blocks: 942367
Attachment #819865 - Flags: review?(martin.thomson) → review+
Blocks: 946348
Comment on attachment 819865 [details] [diff] [review]
0001-Make-DTLS-fingerprint-accessible-from-JS.patch

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

r- for the new[]/delete mismatch. Otherwise, this looks ready to go with one nit.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +752,4 @@
>    // DTLS identity
>    unsigned char fingerprint[DTLS_FINGERPRINT_LENGTH];
>    size_t fingerprint_length;
> +  res = mIdentity->ComputeFingerprint("sha-256",

Martin has volunteered to fix this in a follow-up; see Bug 946348.

@@ +1264,3 @@
>  
>  NS_IMETHODIMP
>  PeerConnectionImpl::GetFingerprint(char** fingerprint)

Just as a minor nit, since we're uncommenting this code: I think the use of NS_ERROR_FAILURE when mIdentity is missing is too generic, and that we should instead be returning NS_ERROR_NOT_INITIALIZED.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
@@ +332,5 @@
> +  {
> +    char *tmp;
> +    GetFingerprint(&tmp);
> +    fingerprint.AssignASCII(tmp);
> +    delete tmp;

delete[] tmp;

I've filed Bug 946377 to fix the other errant deletes in this header file.
Attachment #819865 - Flags: review?(adam) → review-
I've just added brackets to the delete to address Adam's concern.  All the identity patches I've got loaded work with the change, so I think that we're good to go.  (Not sure about BMO protocol at this stage, but I'm going to checkin? :abr and let him tell me that I'm wrong.)
Assignee: nobody → martin.thomson
Attachment #819865 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8342705 - Flags: checkin?(adam)
(In reply to Martin Thomson [:mt] from comment #4)
> (Not sure about BMO protocol at this stage, but I'm going to
> checkin? :abr and let him tell me that I'm wrong.)

Generally, you want to get an r+ before you do a checkin?, since other people (RyanVM, in particular) will sometimes search b.m.o for patches marked "checkin?" and check them in. You should treat setting this flag with pretty much the same reverence you would a repo push.
Attachment #8342705 - Flags: checkin?(adam)
(In reply to Martin Thomson [:mt] from comment #4)
> I've just added brackets to the delete to address Adam's concern.

Thanks. One note for future reference (and this is me personally -- others may feel differently): if I call out a nit in a review, I expect some action; either a code change, or an explanation as to why it is preferable not to fix the nit. Pretty much any explanation will do, but I don't like screaming into the void.

In this case, given that the timeframe on this is critical, the nit is exceedingly minor, and the tree is in a rare open state, I'll land the patch as-is.
Attachment #8342705 - Flags: review+
I'm running a local build for sanity-checking purposes, and plan to push onto m-i before retiring for the evening (assuming the local build doesn't turn up anything fishy).
(In reply to Adam Roach [:abr] from comment #6)
> In this case, given that the timeframe on this is critical, the nit is
> exceedingly minor, and the tree is in a rare open state, I'll land the patch
> as-is.

I'm really sorry, I didn't understand the comment from context and missed it on my second pass.
https://hg.mozilla.org/mozilla-central/rev/4dd987eef431
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.