1010194 - webrtc sipcc/cpr does invalid assumptions about pthread_t

Status: RESOLVED DUPLICATE of bug 1080765

(Core :: WebRTC: Signaling, defect)

Description

[Timo Teräs]

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.

Comment 1

[Timo Teräs]

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

[Mike Hoye [:mhoye]]

This doesn't look like a good first bug to me. Jesup, would you be willing to mentor this?

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

sure

Comment 4

[Mike Hoye [:mhoye]]

thanks!

Comment 5

[Shashank VRSN Sabniveesu]

I see download instructions for 'musl' here: http://www.musl-libc.org/
How exactly should I be compiling to reproduce this bug?

Comment 6

[Timo Teräs]

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

[Shashank VRSN Sabniveesu]

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

Comment 8

[Byron Campen [:bwc]]

FWIW, I think bug 991037 may have actually fixed this, since I ripped (all of?) the threading stuff out of sipcc.

Comment 9

[Byron Campen [:bwc]]

This was fixed when sdparta landed.