dlopen: make error handling thread safe

The dlerror() value is stored in thread local storage and thus checking
the error from go code is not safe because go can schedule goroutines
on any other thread at any time. This means it is possible that between
the dlsym() or dlclose() call the code get moved to another thread and
thus the dlerror() value is not what we expect. Instead because we read
now from another thread it will be an error from a different call.

A test is added which without this fix is able to reproduce the
problem somewhat reliably.

This issue was observed in the podman CI[1] which uses the sdjournal
API.

[1] containers/podman#20569

Signed-off-by: Paul Holzinger <[email protected]>

Author: Luap99

internal/dlopen/dlopen.go

@@ -23,6 +23,7 @@ import "C"
 import (
 	"errors"
 	"fmt"
+	"runtime"
 	"unsafe"
 )
 
@@ -56,6 +57,10 @@ func GetHandle(libs []string) (*LibHandle, error) {
 
 // GetSymbolPointer takes a symbol name and returns a pointer to the symbol.
 func (l *LibHandle) GetSymbolPointer(symbol string) (unsafe.Pointer, error) {
+	// Locking the thread is critical here as the dlerror() is thread local so
+	// go should not reschedule this onto another thread.
+	runtime.LockOSThread()
+	defer runtime.UnlockOSThread()
 	sym := C.CString(symbol)
 	defer C.free(unsafe.Pointer(sym))
 
@@ -71,6 +76,10 @@ func (l *LibHandle) GetSymbolPointer(symbol string) (unsafe.Pointer, error) {
 
 // Close closes a LibHandle.
 func (l *LibHandle) Close() error {
+	// Locking the thread is critical here as the dlerror() is thread local so
+	// go should not reschedule this onto another thread.
+	runtime.LockOSThread()
+	defer runtime.UnlockOSThread()
 	C.dlerror()
 	C.dlclose(l.Handle)
 	e := C.dlerror()

internal/dlopen/dlopen_test.go

@@ -16,6 +16,7 @@ package dlopen
 
 import (
 	"fmt"
+	"sync"
 	"testing"
 )
 
@@ -62,3 +63,42 @@ func TestDlopen(t *testing.T) {
 		}
 	}
 }
+
+// Note this is not a reliable reproducer for the problem.
+// It depends on the fact the it first generates some dlerror() errors
+// by using non existent libraries.
+func TestDlopenThreadSafety(t *testing.T) {
+	libs := []string{
+		"libstrange1.so",
+		"libstrange2.so",
+		"libstrange3.so",
+		"libstrange4.so",
+		"libstrange5.so",
+		"libstrange6.so",
+		"libstrange7.so",
+		"libstrange8.so",
+		"libc.so.6",
+		"libc.so",
+	}
+
+	// 10000 is the default golang thread limit, so adding more will likely fail
+	// but this number is enough to reproduce the issue most of the time for me.
+	count := 10000
+	wg := sync.WaitGroup{}
+	wg.Add(count)
+
+	for i := 0; i < count; i++ {
+		go func() {
+			defer wg.Done()
+			lib, err := GetHandle(libs)
+			if err != nil {
+				t.Errorf("GetHandle failed unexpectedly: %v", err)
+			}
+			_, err = lib.GetSymbolPointer("strlen")
+			if err != nil {
+				t.Errorf("GetSymbolPointer strlen failed unexpectedly: %v", err)
+			}
+		}()
+	}
+	wg.Wait()
+}