[TLS]: Pass a heap-allocated value to pthread_setspecific()

[spevans]

• pthread_setspecific() requires the pointer passed as a value to
  be either NULL or allocated on the heap as the destructor called
  when the thread exits will pass this value to free().

• __dispatch_tsd is already allocated per thread as it is declared
  using __thread so it is stored in the .tbss not the heap.
  Allocate a pointer to hold the address of __dispatch_tsd instead
  of passing the address directly to satisfy pthread_setspecific()
  and still allow _libdispatch_tsd_cleanup() to retrieve the address.

This showed up as an issue on Linux when linking in libdispatch.a into a dynamic executable. It didnt occur when linking in libdispatch.so or when creating a fully static binary (based off of TestFoundation).

The pthread destructor calls free() on the value passed in and this causes a memory error - the destructor doesnt seem to be called in the other two versions.

I dont have a unit test to add here however I will be adding a test to swift-corelibs-foundation that currently triggers this bug. It also shows up in binaries built using the method outlined in swiftlang/swift#9958

Lastly, the manpages on both macOS and Linux are not detailed on whether the value is freed by libpthread, this was mostly found by looking at the glibc source and through experiments. If there is a difference on macOS this PR may need modification.

[MadCoder]

This patch makes absolutely no sense to me. pthread_setspecific() will not call free() as part of the destruction of the TSD value, it will call whatever function you associate with the key. If a free is called it means the function associated with the key calls free and this would be wrong.

[MadCoder]

IOW while I truly believe there's something crashing with the static variant of libdispatch linked in a binary, I believe your patch leaks.

[spevans]

@MadCoder Yeah, I will have another look. there is definitely something calling free() on the value of __dispatch_tsd which seems odd as its not called in _libdispatch_tsd_cleanup() either.

[spevans]

@MadCoder Ok I had another look at this and its due to init constructor ordering.

CoreFoundation/Base.subproj/CFRuntime.c __CFInitialize ultimately calls dispatch_once but if this occurs before src/init.c _libdispatch_init is called then the __dispatch_tsd_key is 0. However libp11-kit owns key 0 and has set its destructor in pthread_key_create() to free().

The ordering is due to libdispatch being a dependancy of libFoundation and with static libraries the dependancy has to come after the library that is dependant on it. This sets the default constructor ordering I believe.

As a test fix I did the following:

$ (cd swift-corelibs-libdispatch/ ; git diff)
diff --git a/src/init.c b/src/init.c
index dea5e87..7faaa24 100644
--- a/src/init.c
+++ b/src/init.c
@@ -33,7 +33,7 @@
 #pragma mark dispatch_init
 
 #if USE_LIBDISPATCH_INIT_CONSTRUCTOR
-DISPATCH_NOTHROW __attribute__((constructor))
+DISPATCH_NOTHROW __attribute__((constructor(9)))
 void
 _libdispatch_init(void);
 

$ (cd swift-corelibs-foundation/ ; git diff)
diff --git a/CoreFoundation/Base.subproj/CFRuntime.c b/CoreFoundation/Base.subproj/CFRuntime.c
index ead676d..35cd491 100644
--- a/CoreFoundation/Base.subproj/CFRuntime.c
+++ b/CoreFoundation/Base.subproj/CFRuntime.c
@@ -966,7 +966,7 @@ pthread_t _CF_pthread_main_thread_np(void) {
 
 
 #if DEPLOYMENT_TARGET_LINUX || DEPLOYMENT_TARGET_FREEBSD
-static void __CFInitialize(void) __attribute__ ((constructor));
+static void __CFInitialize(void) __attribute__ ((constructor(10)));
 static
 #endif
 #if DEPLOYMENT_TARGET_WINDOWS
$ 

which worked but was thinking of a neater fix. Possibly something like the following:

in lib dispatch:
#define LIBDISPATCH_INIT_PRIORITY 9

in CoreFoundation

#if DEPLOYMENT_ENABLE_LIBDISPATCH
#define CFINITIALIZE_PRIORITY (LIBDISPATCH_INIT_PRIORITY + 1)
#endif

The other alternative would be for __CFInitialize to call _libdispatch_init() with a flag in _libdispatch_init() to only allow it to be called once.

[MadCoder]

I strongly dislike relying on constructor ordering across projects.

I think that the fix here is rather that when libdispatch is built statically the libdispatch intializer is not __attribute__((constructor)) and it is the responsibility of whatever projects embeds libdispatch to call it. Here it would be within __CFInitialize that this call should be made.

Incidentally this is exactly how it works on Darwin already, and _libdispatch_init() is only used when USE_LIBDISPATCH_INIT_CONSTRUCTOR is set, which IMO shouldn't be for the static variant. These symbols are already exported:

/* SPI for Libsystem-internal use */
DISPATCH_EXPORT DISPATCH_NOTHROW void libdispatch_init(void);
#if !TARGET_OS_WIN32
DISPATCH_EXPORT DISPATCH_NOTHROW void dispatch_atfork_prepare(void);
DISPATCH_EXPORT DISPATCH_NOTHROW void dispatch_atfork_parent(void);
DISPATCH_EXPORT DISPATCH_NOTHROW void dispatch_atfork_child(void);
#endif

and should be hooked from CF or whichever library you end up hooking libdispatch into. This is exactly what "SPI for Libsystem-internal use" is about, these 4 symbols are about integrating libdispatch inside a larger library which is the case on Darwin (I'm taking shortcuts it's more convoluted than that but true enough for this discussion)

[spevans]

I agree with you about constructor ordering across projects. In this case wouldn't it be easiest to never enable the constructor in libdispatch for either static or dynamic and then always have __CFInitialize call libdispatch_init() ?
Both the libdispatch.so and libdispatch.a built by swift will always be private versions in the swift toolchain. Also I think there is no guarantee that the constructors will be called in the correct order when both libs are dynamic. This also allows mixing a static dispatch with a dynamic Foundation and vice-versa.

One last question. Is there a good reason to have the _libdispatch_init() constructor? Surely any programs / libraries that use libdispatch can just call libdispatch_init() directly and bypass any ordering issues.

[MadCoder]

libdispatch is also used standalone outside of swift and initialization must work here. so no I disagree it should be disabled for the dynamic build. _libdispatch_init() hence must be used for the DSO, but not the static library.

[spevans]

I meant just when building it as part of swift, ie set USE_LIBDISPATCH_INIT_CONSTRUCTOR to 0 as part of the swift build, not removing the constructor.

Alternatively, what about adding a guard into libdispatch_init(), then always call libdispatch_init() from __CFInitialize

diff --git a/src/queue.c b/src/queue.c
index 2406e7e..9461bba 100644
--- a/src/queue.c
+++ b/src/queue.c
@@ -780,6 +780,12 @@ DISPATCH_EXPORT DISPATCH_NOTHROW
 void
 libdispatch_init(void)
 {
+       static int init = 0;
+       if (init != 0) {
+               return;
+       }
+       init = 1;
+

[MadCoder]

I don't like that as it means that it will make distributing swift harder for distributions. When libdispatch is a dynamic library it should use its constructor, so that when linked in C programs it still works without the C program having to initialize it itself.

It;s the only way to allow C/C++ libraries that use dispatch to be used from Swift.

[MadCoder]

BTW I hope that libdispatch as a static library is not the default for Swift builds, exactly for the reason above, we can't quite have two libdispatch's in the same binary, that would end very poorly. It has to be a dynamic module anyway.

[spevans]

The static one will only be linked in on Linux if -static-stdlib is used and if that is the case the dynamic one wont be.

How do you ensure constructor ordering between libFoundation.so and libdispatch.so on Linux because at the moment it may just be working by chance.

[MadCoder]

no there's no chance involved when you have DSOs because the constructors are run in dependency order. The problem exists only within the same object.

[MadCoder]

dead pull request, closing, please reopen with updates if you feel it should be revived