webrtc sipcc/cpr does invalid assumptions about pthread_t

RESOLVED DUPLICATE of bug 1080765

Status

()

RESOLVED DUPLICATE of bug 1080765
5 years ago
4 years ago

People

(Reporter: timo.teras, Unassigned, Mentored)

Tracking

29 Branch
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good next bug])

(Reporter)

Description

5 years ago
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]
(Reporter)

Comment 1

5 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.
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)
(Reporter)

Comment 6

5 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
(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

5 years ago
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.

Updated

4 years ago
Depends on: 1080765
This was fixed when sdparta landed.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: needinfo?(timo.teras)
Flags: needinfo?(rjesup)
Resolution: --- → DUPLICATE
Duplicate of bug: 1080765
You need to log in before you can comment on or make changes to this bug.