Closed
Bug 1010194
Opened 11 years ago
Closed 10 years ago
webrtc sipcc/cpr does invalid assumptions about pthread_t
Categories
(Core :: WebRTC: Signaling, defect)
Tracking
()
RESOLVED
DUPLICATE
of bug 1080765
People
(Reporter: timo.teras, Unassigned, Mentored)
References
Details
(Whiteboard: [good next bug])
User Agent: Mozilla/5.0 (X11; Linux i686; rv:28.0) Gecko/20100101 Firefox/28.0 (Beta/Release)
Build ID: 20140422113055
Steps to reproduce:
Tried to build firefox 29.0.1 against musl c-library on 32-bit platform.
Actual results:
Build failed:
/home/buildozer/aports/main/xulrunner/src/mozilla-release/media/webrtc/signaling/src/sipcc/cpr/linux/cpr_linux_threads.c: In function 'cprCreateThread':
/home/buildozer/aports/main/xulrunner/src/mozilla-release/media/webrtc/signaling/src/sipcc/cpr/linux/cpr_linux_threads.c:95:32: warning: assignment makes integer from pointer without a cast [enabled by default]
threadPtr->u.handleInt = threadId;
^
/home/buildozer/aports/main/xulrunner/src/mozilla-release/media/webrtc/signaling/src/sipcc/cpr/linux/cpr_linux_threads.c: In function 'cprJoinThread':
/home/buildozer/aports/main/xulrunner/src/mozilla-release/media/webrtc/signaling/src/sipcc/cpr/linux/cpr_linux_threads.c:118:5: warning: passing argument 1 of 'pthread_join' makes pointer from integer without a cast [enabled by default]
pthread_join(cprThreadPtr->u.handleInt, NULL);
^
In file included from ../../../../dist/system_wrappers/pthread.h:3:0,
from /home/buildozer/aports/main/xulrunner/src/mozilla-release/media/webrtc/signaling/./src/sipcc/cpr/include/../linux/cpr_linux_ipc.h:9,
from /home/buildozer/aports/main/xulrunner/src/mozilla-release/media/webrtc/signaling/./src/sipcc/cpr/include/cpr_ipc.h:25,
from /home/buildozer/aports/main/xulrunner/src/mozilla-release/media/webrtc/signaling/./src/sipcc/cpr/include/cpr.h:9,
from /home/buildozer/aports/main/xulrunner/src/mozilla-release/media/webrtc/signaling/src/sipcc/cpr/linux/cpr_linux_threads.c:5:
/usr/include/pthread.h:79:5: note: expected 'pthread_t' but argument is of type 'uint64_t'
int pthread_join(pthread_t, void **);
^
/home/buildozer/aports/main/xulrunner/src/mozilla-release/media/webrtc/signaling/src/sipcc/cpr/linux/cpr_linux_threads.c: In function 'cprDestroyThread':
/home/buildozer/aports/main/xulrunner/src/mozilla-release/media/webrtc/signaling/src/sipcc/cpr/linux/cpr_linux_threads.c:146:13: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
if ((pthread_t) cprThreadPtr->u.handleInt == pthread_self()) {
^
/home/buildozer/aports/main/xulrunner/src/mozilla-release/media/webrtc/signaling/src/sipcc/cpr/linux/cpr_linux_threads.c: In function 'cprGetThreadId':
/home/buildozer/aports/main/xulrunner/src/mozilla-release/media/webrtc/signaling/src/sipcc/cpr/linux/cpr_linux_threads.c:213:9: warning: return makes pointer from integer without a cast [enabled by default]
return ((cpr_thread_t *)thread)->u.handleInt;
^
cc1: some warnings being treated as errors
/home/buildozer/aports/main/xulrunner/src/mozilla-release/config/rules.mk:996: recipe for target 'cpr_linux_threads.o' failed
Expected results:
Successful build. The underlying root cause is that the code assumes pthread_t to be an integral typedef or 64-bit wide data type.
Standards do not strictly define underlying pthread_t type:
http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_types.h.html
In practice C++ ABI require pthread_t to be unsigned long. But implementation might define pthread_t to be a pointer in C (this is what musl does) as they have equal bitwidths.
The errors are caused by invalid implicit casts. Minimal fix is:
diff -ru mozilla-release/media.orig/webrtc/signaling/src/sipcc/cpr/include/cpr_threads.h mozilla-release/media/webrtc/signaling/src/sipcc/cpr/include/cpr_threads.h
--- mozilla-release/media.orig/webrtc/signaling/src/sipcc/cpr/include/cpr_threads.h 2014-05-07 01:56:10.000000000 -0300
+++ mozilla-release/media/webrtc/signaling/src/sipcc/cpr/include/cpr_threads.h 2014-05-14 14:56:45.938648384 -0300
@@ -30,7 +30,7 @@
uint32_t threadId;
union {
void *handlePtr;
- uint64_t handleInt;
+ unsigned long handleInt;
} u;
} cpr_thread_t;
But that is really a kludge at best. It would best to use pthread_t explicitly.
Updated•11 years ago
|
Status: UNCONFIRMED → NEW
Component: Untriaged → WebRTC: Signaling
Ever confirmed: true
Product: Firefox → Core
Updated•11 years ago
|
Whiteboard: [good-first-bug]
Reporter | ||
Comment 1•11 years ago
|
||
As additional notes. Firefox 28.0 did not have this issue.
Additionally, pthread_t's should not be compared directly with ==. pthread_equal() should be used instead.
Comment 2•11 years ago
|
||
This doesn't look like a good first bug to me. Jesup, would you be willing to mentor this?
Flags: needinfo?(rjesup)
Whiteboard: [good-first-bug] → [good next bug]
Comment 5•10 years ago
|
||
I see download instructions for 'musl' here: http://www.musl-libc.org/
How exactly should I be compiling to reproduce this bug?
Flags: needinfo?(rjesup)
Reporter | ||
Comment 6•10 years ago
|
||
There are other bugs in firefox source code base, so it will not build against musl unpatched. This is one of the many issues. I suggest to just fix the source code to not do casts from pthread_t to other types, and treat it as opaque data type.
If you are still interested to build against musl, the Alpine Linux patches are at:
http://git.alpinelinux.org/cgit/aports/tree/main/xulrunner
Comment 7•10 years ago
|
||
(In reply to Timo Teräs from comment #6)
> I suggest to just
> fix the source code to not do casts from pthread_t to other types, and treat
> it as opaque data type.
>
Where are these casts happening? I see usages like 'pthread_t threadId;' in cpr_*_threads.c files
So, the changes mentioned in comment0 alone would do? What about the suggestion at its end? (asking for a 'pthread_t' directly than casting)
Flags: needinfo?(timo.teras)
Updated•10 years ago
|
Mentor: rjesup
Whiteboard: [good next bug][mentor=rjesup] → [good next bug]
Comment 8•10 years ago
|
||
FWIW, I think bug 991037 may have actually fixed this, since I ripped (all of?) the threading stuff out of sipcc.
Comment 9•10 years ago
|
||
This was fixed when sdparta landed.
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: needinfo?(timo.teras)
Flags: needinfo?(rjesup)
Resolution: --- → DUPLICATE
You need to log in
before you can comment on or make changes to this bug.
Description
•