Closed Bug 823338 Opened 7 years ago Closed 6 years ago

Minor bug in media/mtransport/transportlayerdtls.cpp

Categories

(Core :: WebRTC: Networking, defect, P3)

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: kolya, Assigned: kolya)

Details

(Whiteboard: [WebRTC] [blocking-webrtc-])

Attachments

(1 file)

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.17 (KHTML, like Gecko) Chrome/24.0.1312.40 Safari/537.17

Steps to reproduce:

I looked for various bugs in the Mozilla source code.


Actual results:

I found a minor issue in media/mtransport/transportlayerdtls.cpp, where TransportLayerDtls::GetClientAuthDataHook() incorrectly checks for out-of-memory conditions.  I've attached a fairly obvious patch.
Component: Untriaged → WebRTC: Networking
Product: Firefox → Core
QA Contact: jsmith
Attachment #694171 - Flags: review?(ekr)
Comment on attachment 694171 [details] [diff] [review]
Patch for TransportLayerDtls::GetClientAuthDataHook()

lgtm
Attachment #694171 - Flags: review?(ekr) → review+
Comment on attachment 694171 [details] [diff] [review]
Patch for TransportLayerDtls::GetClientAuthDataHook()

I think these actually may not be able to fail, because they just increment a refct...
Attachment #694171 - Flags: feedback?(bsmith)
(In reply to Eric Rescorla (:ekr) from comment #3)
> I think these actually may not be able to fail, because they just increment
> a refct...

True for CERT_DupCertificate but not for SECKEY_CopyPrivateKey. However, I bet libbsl itself sanity checks the result. (Worth checking, to see if Nickolai qualifies for the bug bounty.)

Thanks for the patch, Nickolai!.
    The answer appears to be that it checks it.

    /* check what the callback function returned */
        if ((!ss->ssl3.clientCertificate) || (!ss->ssl3.clientPrivateKey)) {
            /* we are missing either the key or cert */
            if (ss->ssl3.clientCertificate) {
                /* got a cert, but no key - free it */
                CERT_DestroyCertificate(ss->ssl3.clientCertificate);
                ss->ssl3.clientCertificate = NULL;
            }
            if (ss->ssl3.clientPrivateKey) {
                /* got a key, but no cert - free it */
                SECKEY_DestroyPrivateKey(ss->ssl3.clientPrivateKey);
                ss->ssl3.clientPrivateKey = NULL;
            }
            goto send_no_certificate;
        }
So, I take it this should be RESOLVED-INVALID?
Flags: needinfo?(bsmith)
Well, it's still a defect, since we're not correctly returning an error when we
should. And this patch looked correct when I looked at it before.
Assignee: nobody → ekr
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(bsmith)
Priority: -- → P3
Whiteboard: [WebRTC] [blocking-webrtc-]
Sorry, assigning to the person who has the patch up for review.   Just waiting on feedback from bsmith
Assignee: ekr → kolya
Comment on attachment 694171 [details] [diff] [review]
Patch for TransportLayerDtls::GetClientAuthDataHook()

This should be uplifted to aurora if WebRTC is enabled in Aurora.
Attachment #694171 - Flags: feedback?(bsmith) → feedback+
Attachment #694171 - Attachment is patch: true
Attachment #694171 - Flags: checkin?(ekr)
Comment on attachment 694171 [details] [diff] [review]
Patch for TransportLayerDtls::GetClientAuthDataHook()

Just use the checkin-needed keyword in the future please.
Attachment #694171 - Flags: checkin?(ekr) → checkin+
https://hg.mozilla.org/integration/mozilla-inbound/rev/b26ac05ab0b0

Thanks for the patch, Nickolai! One request, for future patches, please make sure that you have Mercurial configured to generate patches will all the necessary commit information. It makes life much easier for those landing on your behalf. Thanks!
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
https://hg.mozilla.org/mozilla-central/rev/b26ac05ab0b0
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.