Closed Bug 919815 Opened 9 years ago Closed 8 years ago

cpr_win_ipc.c not 64-bit safe

Categories

(Core :: WebRTC: Signaling, defect)

x86_64
Windows 8
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: vlad, Assigned: vlad)

References

Details

(Whiteboard: [webrtc])

Attachments

(2 files)

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.
Clearly wrong.  Only happens on 64-bit Windows builds, which we're not doing by default (anymore).  Should be a simple fix.
Assignee: nobody → ethanhugg
Whiteboard: [webrtc]
Target Milestone: --- → mozilla27
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).
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;

?
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.
I patched this up last night to get further in the tests overnight; seems to work fine.
Attachment #809197 - Flags: review?(ethanhugg)
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.
Attachment #809197 - Flags: review?(ethanhugg) → review+
https://hg.mozilla.org/mozilla-central/rev/d4e4cc29a3e4
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.