Last Comment Bug 823338 - Minor bug in media/mtransport/transportlayerdtls.cpp
: Minor bug in media/mtransport/transportlayerdtls.cpp
Status: RESOLVED FIXED
[WebRTC] [blocking-webrtc-]
:
Product: Core
Classification: Components
Component: WebRTC: Networking (show other bugs)
: Trunk
: x86_64 Linux
: P3 normal (vote)
: mozilla25
Assigned To: Nickolai Zeldovich
: Jason Smith [:jsmith]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-12-19 17:37 PST by Nickolai Zeldovich
Modified: 2013-07-24 20:01 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch for TransportLayerDtls::GetClientAuthDataHook() (642 bytes, patch)
2012-12-19 17:37 PST, Nickolai Zeldovich
ekr: review+
brian: feedback+
ryanvm: checkin+
Details | Diff | Splinter Review

Description Nickolai Zeldovich 2012-12-19 17:37:15 PST
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.
Comment 1 Nickolai Zeldovich 2012-12-19 17:37:50 PST
Created attachment 694171 [details] [diff] [review]
Patch for TransportLayerDtls::GetClientAuthDataHook()
Comment 2 Eric Rescorla (:ekr) 2012-12-20 14:28:37 PST
Comment on attachment 694171 [details] [diff] [review]
Patch for TransportLayerDtls::GetClientAuthDataHook()

lgtm
Comment 3 Eric Rescorla (:ekr) 2012-12-20 14:31:08 PST
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...
Comment 4 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-12-20 15:06:04 PST
(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!.
Comment 5 Eric Rescorla (:ekr) 2012-12-20 15:10:40 PST
    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;
        }
Comment 6 Randell Jesup [:jesup] 2013-01-05 21:07:54 PST
So, I take it this should be RESOLVED-INVALID?
Comment 7 Eric Rescorla (:ekr) 2013-01-05 21:17:56 PST
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.
Comment 8 Randell Jesup [:jesup] 2013-01-09 09:30:28 PST
Sorry, assigning to the person who has the patch up for review.   Just waiting on feedback from bsmith
Comment 9 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2013-02-22 16:23:29 PST
Comment on attachment 694171 [details] [diff] [review]
Patch for TransportLayerDtls::GetClientAuthDataHook()

This should be uplifted to aurora if WebRTC is enabled in Aurora.
Comment 10 Ryan VanderMeulen [:RyanVM] 2013-07-24 06:06:06 PDT
Comment on attachment 694171 [details] [diff] [review]
Patch for TransportLayerDtls::GetClientAuthDataHook()

Just use the checkin-needed keyword in the future please.
Comment 11 Ryan VanderMeulen [:RyanVM] 2013-07-24 06:11:28 PDT
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
Comment 12 Wes Kocher (:KWierso) 2013-07-24 20:01:17 PDT
https://hg.mozilla.org/mozilla-central/rev/b26ac05ab0b0

Note You need to log in before you can comment on or make changes to this bug.