Closed Bug 1010194 Opened 10 years ago Closed 9 years ago

webrtc sipcc/cpr does invalid assumptions about pthread_t


(Core :: WebRTC: Signaling, defect)

29 Branch
Not set





(Reporter: timo.teras, Unassigned, Mentored)



(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/ 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:

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.
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]
Flags: needinfo?(rjesup)
Whiteboard: [good next bug] → [good next bug][mentor=rjesup]
I see download instructions for 'musl' here:
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:
(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.
Closed: 9 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.