Closed
Bug 919815
Opened 11 years ago
Closed 11 years ago
cpr_win_ipc.c not 64-bit safe
Categories
(Core :: WebRTC: Signaling, defect)
Tracking
()
RESOLVED
FIXED
mozilla27
People
(Reporter: vlad, Assigned: vlad)
References
Details
(Whiteboard: [webrtc])
Attachments
(2 files)
1.97 KB,
patch
|
ehugg
:
review+
|
Details | Diff | Splinter Review |
1.97 KB,
patch
|
Details | Diff | Splinter Review |
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•11 years ago
|
||
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
Assignee | ||
Comment 2•11 years ago
|
||
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).
Assignee | ||
Comment 3•11 years ago
|
||
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•11 years ago
|
||
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.
Assignee | ||
Comment 5•11 years ago
|
||
I patched this up last night to get further in the tests overnight; seems to work fine.
Attachment #809197 -
Flags: review?(ethanhugg)
Comment 6•11 years ago
|
||
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+
Assignee | ||
Comment 7•11 years ago
|
||
Assignee: ethanhugg → vladimir
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 8•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4e4cc29a3e4
Keywords: checkin-needed
Comment 9•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d4e4cc29a3e4
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•