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)

29 Branch
x86
Linux
defect
Not set
normal

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.
Status: UNCONFIRMED → NEW
Component: Untriaged → WebRTC: Signaling
Ever confirmed: true
Product: Firefox → Core
Whiteboard: [good-first-bug]
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.
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]
sure
Flags: needinfo?(rjesup)
thanks!
Whiteboard: [good next bug] → [good next bug][mentor=rjesup]
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)
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
(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)
Mentor: rjesup
Whiteboard: [good next bug][mentor=rjesup] → [good next bug]
FWIW, I think bug 991037 may have actually fixed this, since I ripped (all of?) the threading stuff out of sipcc.
Depends on: sdparta
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.