919815 - cpr_win_ipc.c not 64-bit safe

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

mochitest-plain crash on a 64-bit windows build in test_dataChannel_noOffer.html

It looks like cpr_win_ipc.c passes around a pointer using uint32_t, with the MSG_BUF event:

    /* Save the address of the message */
    memcpy(&sendMsg->msgPtr, &msg, sizeof(uint32_t));

    /* Allow the usrPtr to be optional */
    if (usrPtr) {
        sendMsg->usrPtr = *usrPtr;
    }

and later:

        case MSG_BUF:
            rcvMsg = (struct msgbuffer *)msg.wParam;
            memcpy(&bufferPtr, &rcvMsg->msgPtr, sizeof(uint32_t));

    ...
    return (void *)bufferPtr;

Bad times.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Clearly wrong.  Only happens on 64-bit Windows builds, which we're not doing by default (anymore).  Should be a simple fix.

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Yep, discovered while looking at test failures in preparation for getting Win64 into a releasable state :)  We've always done builds fwiw, we just turned off automated testing (or support, really).

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

From looking at the code -- all that memcpy goop seems bizarre. Is this not just:

  sendMsg->msgPtr = msg;

and

  void *bufferPtr = 0;  // it's defined as a uint32_t now
  bufferPtr = rcvMsg->msgPtr;

?

Comment 4

[Ethan Hugg [:ehugg]]

Yes the cprGetMessage does the same type of memcpy.  No good reason to do this that I can think of.  I'm going to change these to void* and assignments like you suggest and give it a try.

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: remove unnecessary memcpy/casts

I patched this up last night to get further in the tests overnight; seems to work fine.

Comment 6

[Ethan Hugg [:ehugg]]

Comment on attachment 809197 [details] [diff] [review]
remove unnecessary memcpy/casts

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

Looks good to me.

::: media/webrtc/signaling/src/sipcc/cpr/win32/cpr_win_ipc.c
@@ +189,4 @@
>  {
>      static const char fname[] = "cprGetMessage";
>      struct msgbuffer *rcvMsg = (struct msgbuffer *)rcvBuffer;
> +    void *bufferPtr = 0;

void *bufferPtr = NULL; 
would be the style in this area.

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: patch for checkin (with NULL fix)

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d4e4cc29a3e4

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d4e4cc29a3e4