From 2e4948c38c464f1d29f9ba2fe4a2935672c522ee Mon Sep 17 00:00:00 2001 From: Jakub Adam Date: Tue, 19 May 2015 15:13:32 +0200 Subject: [PATCH 1/2] Make remdesk_virtual_channel_write() static The function with the same name is defined in multiple libraries (libfreerdp-shadow.so, libfreerdp-server.so), which might confuse the dynamic linker at runtime, binding a wrong version of the function and thus causing segmentation faults. Since remdesk_virtual_channel_write()s aren't used outside the files they are defined in, we can declare them static to make them invisible to the linker. --- channels/remdesk/client/remdesk_main.c | 2 +- channels/remdesk/server/remdesk_main.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/channels/remdesk/client/remdesk_main.c b/channels/remdesk/client/remdesk_main.c index 53ea9eb5a..7df6d634e 100644 --- a/channels/remdesk/client/remdesk_main.c +++ b/channels/remdesk/client/remdesk_main.c @@ -38,7 +38,7 @@ RemdeskClientContext* remdesk_get_client_interface(remdeskPlugin* remdesk) return pInterface; } -int remdesk_virtual_channel_write(remdeskPlugin* remdesk, wStream* s) +static int remdesk_virtual_channel_write(remdeskPlugin* remdesk, wStream* s) { UINT32 status = 0; diff --git a/channels/remdesk/server/remdesk_main.c b/channels/remdesk/server/remdesk_main.c index fa4bde6af..51d276ad3 100644 --- a/channels/remdesk/server/remdesk_main.c +++ b/channels/remdesk/server/remdesk_main.c @@ -27,7 +27,7 @@ #include "remdesk_main.h" -int remdesk_virtual_channel_write(RemdeskServerContext* context, wStream* s) +static int remdesk_virtual_channel_write(RemdeskServerContext* context, wStream* s) { BOOL status; ULONG BytesWritten = 0; From 73888a57c2c5ef0e4830e59a971103cee3b16c17 Mon Sep 17 00:00:00 2001 From: Jakub Adam Date: Thu, 21 May 2015 08:32:51 +0200 Subject: [PATCH 2/2] Ensure threads have finished using a barrier before releasing it MSDN documentation says it is ensured that all threads in the barrier have finished using it before allowing the barrier to be released in DeleteSynchronizationBarrier(). The winpr re-implementation wasn't keeping to that requirement, which was causing occasional crashes when shadow client tried to access already freed barrier structure. The crash was occuring in winpr_Handle_cleanup() after finished waiting on a barrier's event. --- winpr/libwinpr/synch/barrier.c | 5 +++ winpr/libwinpr/synch/test/TestSynchBarrier.c | 33 +++++++++++++++++++- 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/winpr/libwinpr/synch/barrier.c b/winpr/libwinpr/synch/barrier.c index f8d75cb5d..225df0837 100644 --- a/winpr/libwinpr/synch/barrier.c +++ b/winpr/libwinpr/synch/barrier.c @@ -145,6 +145,8 @@ BOOL WINAPI EnterSynchronizationBarrier(LPSYNCHRONIZATION_BARRIER lpBarrier, DWO status = TRUE; } + InterlockedDecrement(&(pBarrier->count)); + return status; } @@ -165,6 +167,9 @@ BOOL WINAPI DeleteSynchronizationBarrier(LPSYNCHRONIZATION_BARRIER lpBarrier) if (!pBarrier) return TRUE; + while (InterlockedCompareExchange(&pBarrier->count, 0, 0) != 0) + Sleep(100); + CloseHandle(pBarrier->event); free(pBarrier); diff --git a/winpr/libwinpr/synch/test/TestSynchBarrier.c b/winpr/libwinpr/synch/test/TestSynchBarrier.c index 9f804eea0..9a7529e74 100644 --- a/winpr/libwinpr/synch/test/TestSynchBarrier.c +++ b/winpr/libwinpr/synch/test/TestSynchBarrier.c @@ -3,6 +3,8 @@ #include #include +#include "../synch.h" + static int g_Count; static HANDLE g_Event; static CRITICAL_SECTION g_Lock; @@ -29,10 +31,19 @@ static void* test_synch_barrier_thread_func(void* arg) return NULL; } +static void* barrier_deleter_thread_func(void* arg) +{ + /* Blocks until all threads are released from the barrier. */ + DeleteSynchronizationBarrier(&g_Barrier); + + return NULL; +} + int TestSynchBarrier(int argc, char* argv[]) { int index; HANDLE threads[5]; + HANDLE deleter_thread = NULL; g_Count = 0; @@ -65,10 +76,30 @@ int TestSynchBarrier(int argc, char* argv[]) printf("%s: CreateThread failed for thread #%d. GetLastError() = 0x%08x\n", __FUNCTION__, index, GetLastError()); while (index) CloseHandle(threads[--index]); + CloseHandle(deleter_thread); DeleteCriticalSection(&g_Lock); CloseHandle(g_Event); return -1; } + + if (index == 0) + { + /* Make sure first thread has already entered the barrier... */ + while (((WINPR_BARRIER*) g_Barrier.Reserved3[0])->count == 0) + Sleep(100); + + /* Now spawn the deleter thread. */ + if (!(deleter_thread = CreateThread(NULL, 0, (LPTHREAD_START_ROUTINE) + barrier_deleter_thread_func, NULL, 0, NULL))) + { + printf("%s: CreateThread failed for deleter thread. GetLastError() = 0x%08x\n", __FUNCTION__, GetLastError()); + while (index) + CloseHandle(threads[--index]); + DeleteCriticalSection(&g_Lock); + CloseHandle(g_Event); + return -1; + } + } } WaitForSingleObject(g_Event, INFINITE); @@ -83,7 +114,7 @@ int TestSynchBarrier(int argc, char* argv[]) CloseHandle(threads[index]); } - DeleteSynchronizationBarrier(&g_Barrier); + CloseHandle(deleter_thread); DeleteCriticalSection(&g_Lock);