[PATCH 1/4] quartz/systemclock: Avoid using the sink structure pointer as a cookie.

[Zebediah Figura]

The same pointer address may be allocated to multiple different structures.

Signed-off-by: Zebediah Figura 
---
 dlls/quartz/systemclock.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/dlls/quartz/systemclock.c b/dlls/quartz/systemclock.c
index 0cc6d87f84..f41cdbe534 100644
--- a/dlls/quartz/systemclock.c
+++ b/dlls/quartz/systemclock.c
@@ -26,11 +26,14 @@

WINE_DEFAULT_DEBUG_CHANNEL(quartz);

+static int cookie_counter;
+
 struct advise_sink
 {
     struct list entry;
     HANDLE handle;
     REFERENCE_TIME due_time, period;
+    int cookie;
 };

typedef struct SystemClockImpl {
@@ -228,6 +231,7 @@ static HRESULT WINAPI SystemClockImpl_AdviseTime(IReferenceClock *iface,
     sink->handle = (HANDLE)event;
     sink->due_time = base + offset;
     sink->period = 0;
+    sink->cookie = InterlockedIncrement(&cookie_counter);

EnterCriticalSection(&clock->cs);
     insert_advise_sink(sink, &clock->single_sinks);
@@ -235,7 +239,7 @@ static HRESULT WINAPI SystemClockImpl_AdviseTime(IReferenceClock *iface,

notify_thread(clock);

-    *cookie = (DWORD_PTR)sink;
+    *cookie = sink->cookie;
     return S_OK;
 }

@@ -263,6 +267,7 @@ static HRESULT WINAPI SystemClockImpl_AdvisePeriodic(IReferenceClock* iface,
     sink->handle = (HANDLE)semaphore;
     sink->due_time = start;
     sink->period = period;
+    sink->cookie = InterlockedIncrement(&cookie_counter);

EnterCriticalSection(&clock->cs);
     insert_advise_sink(sink, &clock->periodic_sinks);
@@ -270,7 +275,7 @@ static HRESULT WINAPI SystemClockImpl_AdvisePeriodic(IReferenceClock* iface,

notify_thread(clock);

-    *cookie = (DWORD_PTR)sink;
+    *cookie = sink->cookie;
     return S_OK;
 }

@@ -285,7 +290,7 @@ static HRESULT WINAPI SystemClockImpl_Unadvise(IReferenceClock *iface, DWORD_PTR

LIST_FOR_EACH_ENTRY(sink, &clock->single_sinks, struct advise_sink, entry)
     {
-        if (sink == (struct advise_sink *)cookie)
+        if (sink->cookie == cookie)
         {
             list_remove(&sink->entry);
             heap_free(sink);
@@ -296,7 +301,7 @@ static HRESULT WINAPI SystemClockImpl_Unadvise(IReferenceClock *iface, DWORD_PTR

LIST_FOR_EACH_ENTRY(sink, &clock->periodic_sinks, struct advise_sink, entry)
     {
-        if (sink == (struct advise_sink *)cookie)
+        if (sink->cookie == cookie)
         {
             list_remove(&sink->entry);
             heap_free(sink);

[PATCH 2/4] quartz/systemclock: Store all the sinks in a single list.

[Zebediah Figura]

Signed-off-by: Zebediah Figura 
---
 dlls/quartz/systemclock.c | 116 +++++++++++++-------------------------
 1 file changed, 39 insertions(+), 77 deletions(-)

diff --git a/dlls/quartz/systemclock.c b/dlls/quartz/systemclock.c
index f41cdbe534..f22c781a3b 100644
--- a/dlls/quartz/systemclock.c
+++ b/dlls/quartz/systemclock.c
@@ -45,8 +45,7 @@ typedef struct SystemClockImpl {
     REFERENCE_TIME last_time;
     CRITICAL_SECTION cs;

-    /* These lists are ordered by expiration time (soonest first). */
-    struct list single_sinks, periodic_sinks;
+    struct list sinks;
 } SystemClockImpl;

static inline SystemClockImpl *impl_from_IReferenceClock(IReferenceClock *iface)
@@ -54,73 +53,48 @@ static inline SystemClockImpl *impl_from_IReferenceClock(IReferenceClock *iface)
     return CONTAINING_RECORD(iface, SystemClockImpl, IReferenceClock_iface);
 }

-static void insert_advise_sink(struct advise_sink *sink, struct list *queue)
+static DWORD WINAPI SystemClockAdviseThread(void *param)
 {
-    REFERENCE_TIME due_time = sink->due_time + sink->period;
-    struct advise_sink *cursor;
+    SystemClockImpl *clock = param;
+    struct advise_sink *sink, *cursor;
+    REFERENCE_TIME current_time;
+    DWORD timeout = INFINITE;
+    HANDLE handles[2] = {clock->stop_event, clock->notify_event};

-    LIST_FOR_EACH_ENTRY(cursor, queue, struct advise_sink, entry)
-    {
-        if (cursor->due_time + cursor->period > due_time)
-        {
-            list_add_before(&cursor->entry, &sink->entry);
-            return;
-        }
-    }
-
-    list_add_tail(queue, &sink->entry);
-}
-
-static DWORD WINAPI SystemClockAdviseThread(LPVOID lpParam) {
-  SystemClockImpl* This = lpParam;
-  struct advise_sink *sink, *cursor;
-  struct list *entry;
-  DWORD timeOut = INFINITE;
-  REFERENCE_TIME curTime;
-  HANDLE handles[2] = {This->stop_event, This->notify_event};
-
-  TRACE("(%p): Main Loop\n", This);
+    TRACE("Starting advise thread for clock %p.\n", clock);

-  while (TRUE) {
-    EnterCriticalSection(&This->cs);
-
-    curTime = GetTickCount64() * 10000;
-
-    /** First SingleShots Advice: sorted list */
-    LIST_FOR_EACH_ENTRY_SAFE(sink, cursor, &This->single_sinks, struct advise_sink, entry)
+    for (;;)
     {
-      if (sink->due_time + sink->period > curTime)
-        break;
+        EnterCriticalSection(&clock->cs);

-      SetEvent(sink->handle);
-      list_remove(&sink->entry);
-      heap_free(sink);
-    }
+        current_time = GetTickCount64() * 10000;

-    if ((entry = list_head(&This->single_sinks)))
-    {
-      sink = LIST_ENTRY(entry, struct advise_sink, entry);
-      timeOut = (sink->due_time + sink->period - curTime) / 10000;
-    }
-    else timeOut = INFINITE;
-
-    /** Now Periodics Advice: semi sorted list (sort cannot be used) */
-    LIST_FOR_EACH_ENTRY(sink, &This->periodic_sinks, struct advise_sink, entry)
-    {
-      if (sink->due_time <= curTime)
-      {
-        DWORD periods = ((curTime - sink->due_time) / sink->period) + 1;
-        ReleaseSemaphore(sink->handle, periods, NULL);
-        sink->due_time += periods * sink->period;
-      }
-      timeOut = min(timeOut, (sink->due_time - curTime) / 10000);
-    }
+        LIST_FOR_EACH_ENTRY_SAFE(sink, cursor, &clock->sinks, struct advise_sink, entry)
+        {
+            if (sink->due_time <= current_time)
+            {
+                if (sink->period)
+                {
+                    DWORD periods = ((current_time - sink->due_time) / sink->period) + 1;
+                    ReleaseSemaphore(sink->handle, periods, NULL);
+                    sink->due_time += periods * sink->period;
+                }
+                else
+                {
+                    SetEvent(sink->handle);
+                    list_remove(&sink->entry);
+                    heap_free(sink);
+                }
+            }
+
+            timeout = min(timeout, (sink->due_time - current_time) / 10000);
+        }

-    LeaveCriticalSection(&This->cs);
+        LeaveCriticalSection(&clock->cs);

-    if (WaitForMultipleObjects(2, handles, FALSE, timeOut) == 0)
-        return 0;
-  }
+        if (WaitForMultipleObjects(2, handles, FALSE, timeout) == 0)
+            return 0;
+    }
 }

static void notify_thread(SystemClockImpl *clock)
@@ -234,7 +208,7 @@ static HRESULT WINAPI SystemClockImpl_AdviseTime(IReferenceClock *iface,
     sink->cookie = InterlockedIncrement(&cookie_counter);

EnterCriticalSection(&clock->cs);
-    insert_advise_sink(sink, &clock->single_sinks);
+    list_add_tail(&clock->sinks, &sink->entry);
     LeaveCriticalSection(&clock->cs);

notify_thread(clock);
@@ -270,7 +244,7 @@ static HRESULT WINAPI SystemClockImpl_AdvisePeriodic(IReferenceClock* iface,
     sink->cookie = InterlockedIncrement(&cookie_counter);

EnterCriticalSection(&clock->cs);
-    insert_advise_sink(sink, &clock->periodic_sinks);
+    list_add_tail(&clock->sinks, &sink->entry);
     LeaveCriticalSection(&clock->cs);

notify_thread(clock);
@@ -288,18 +262,7 @@ static HRESULT WINAPI SystemClockImpl_Unadvise(IReferenceClock *iface, DWORD_PTR

EnterCriticalSection(&clock->cs);

-    LIST_FOR_EACH_ENTRY(sink, &clock->single_sinks, struct advise_sink, entry)
-    {
-        if (sink->cookie == cookie)
-        {
-            list_remove(&sink->entry);
-            heap_free(sink);
-            LeaveCriticalSection(&clock->cs);
-            return S_OK;
-        }
-    }
-
-    LIST_FOR_EACH_ENTRY(sink, &clock->periodic_sinks, struct advise_sink, entry)
+    LIST_FOR_EACH_ENTRY(sink, &clock->sinks, struct advise_sink, entry)
     {
         if (sink->cookie == cookie)
         {
@@ -339,8 +302,7 @@ HRESULT QUARTZ_CreateSystemClock(IUnknown *outer, void **out)
     }

object->IReferenceClock_iface.lpVtbl = &SystemClock_Vtbl;
-    list_init(&object->single_sinks);
-    list_init(&object->periodic_sinks);
+    list_init(&object->sinks);
     InitializeCriticalSection(&object->cs);
     object->cs.DebugInfo->Spare[0] = (DWORD_PTR)(__FILE__ ": SystemClockImpl.cs");

[PATCH 3/4] quartz/systemclock: Clean up the remaining IUnknown methods.

[Zebediah Figura]

Signed-off-by: Zebediah Figura 
---
 dlls/quartz/systemclock.c | 42 ++++++++++++++++++++-------------------
 1 file changed, 22 insertions(+), 20 deletions(-)

diff --git a/dlls/quartz/systemclock.c b/dlls/quartz/systemclock.c
index f22c781a3b..47f0daddbc 100644
--- a/dlls/quartz/systemclock.c
+++ b/dlls/quartz/systemclock.c
@@ -108,29 +108,31 @@ static void notify_thread(SystemClockImpl *clock)
     SetEvent(clock->notify_event);
 }

-static ULONG WINAPI SystemClockImpl_AddRef(IReferenceClock* iface) {
-  SystemClockImpl *This = impl_from_IReferenceClock(iface);
-  ULONG ref = InterlockedIncrement(&This->ref);
-
-  TRACE("(%p): AddRef from %d\n", This, ref - 1);
+static HRESULT WINAPI SystemClockImpl_QueryInterface(IReferenceClock *iface, REFIID iid, void **out)
+{
+    SystemClockImpl *clock = impl_from_IReferenceClock(iface);
+    TRACE("clock %p, iid %s, out %p.\n", clock, debugstr_guid(iid), out);
+  
+    if (IsEqualGUID(iid, &IID_IUnknown) || IsEqualGUID(iid, &IID_IReferenceClock))
+    {
+        IReferenceClock_AddRef(iface);
+        *out = iface;
+        return S_OK;
+    }

-  return ref;
+    WARN("%s not implemented, returning E_NOINTERFACE.\n", debugstr_guid(iid));
+    *out = NULL;
+    return E_NOINTERFACE;
 }

-static HRESULT WINAPI SystemClockImpl_QueryInterface(IReferenceClock* iface, REFIID riid, void** ppobj) {
-  SystemClockImpl *This = impl_from_IReferenceClock(iface);
-  TRACE("(%p, %s,%p)\n", This, debugstr_guid(riid), ppobj);
-  
-  if (IsEqualIID (riid, &IID_IUnknown) || 
-      IsEqualIID (riid, &IID_IReferenceClock)) {
-    SystemClockImpl_AddRef(iface);
-    *ppobj = &This->IReferenceClock_iface;
-    return S_OK;
-  }
-  
-  *ppobj = NULL;
-  WARN("(%p, %s,%p): not found\n", This, debugstr_guid(riid), ppobj);
-  return E_NOINTERFACE;
+static ULONG WINAPI SystemClockImpl_AddRef(IReferenceClock *iface)
+{
+    SystemClockImpl *clock = impl_from_IReferenceClock(iface);
+    ULONG refcount = InterlockedIncrement(&clock->ref);
+
+    TRACE("%p increasing refcount to %u.\n", clock, refcount);
+
+    return refcount;
 }

static ULONG WINAPI SystemClockImpl_Release(IReferenceClock *iface)

[PATCH 4/4] quartz/systemclock: Get rid of the SystemClockImpl typedef.

[Zebediah Figura]

Signed-off-by: Zebediah Figura 
---
 dlls/quartz/systemclock.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/dlls/quartz/systemclock.c b/dlls/quartz/systemclock.c
index 47f0daddbc..239155ea78 100644
--- a/dlls/quartz/systemclock.c
+++ b/dlls/quartz/systemclock.c
@@ -36,7 +36,8 @@ struct advise_sink
     int cookie;
 };

-typedef struct SystemClockImpl {
+struct system_clock
+{
     IReferenceClock IReferenceClock_iface;
     LONG ref;

@@ -46,16 +47,16 @@ typedef struct SystemClockImpl {
     CRITICAL_SECTION cs;

struct list sinks;
-} SystemClockImpl;
+};

-static inline SystemClockImpl *impl_from_IReferenceClock(IReferenceClock *iface)
+static inline struct system_clock *impl_from_IReferenceClock(IReferenceClock *iface)
 {
-    return CONTAINING_RECORD(iface, SystemClockImpl, IReferenceClock_iface);
+    return CONTAINING_RECORD(iface, struct system_clock, IReferenceClock_iface);
 }

static DWORD WINAPI SystemClockAdviseThread(void *param)
 {
-    SystemClockImpl *clock = param;
+    struct system_clock *clock = param;
     struct advise_sink *sink, *cursor;
     REFERENCE_TIME current_time;
     DWORD timeout = INFINITE;
@@ -97,7 +98,7 @@ static DWORD WINAPI SystemClockAdviseThread(void *param)
     }
 }

-static void notify_thread(SystemClockImpl *clock)
+static void notify_thread(struct system_clock *clock)
 {
     if (!InterlockedCompareExchange(&clock->thread_created, TRUE, FALSE))
     {
@@ -110,7 +111,7 @@ static void notify_thread(SystemClockImpl *clock)

static HRESULT WINAPI SystemClockImpl_QueryInterface(IReferenceClock *iface, REFIID iid, void **out)
 {
-    SystemClockImpl *clock = impl_from_IReferenceClock(iface);
+    struct system_clock *clock = impl_from_IReferenceClock(iface);
     TRACE("clock %p, iid %s, out %p.\n", clock, debugstr_guid(iid), out);

if (IsEqualGUID(iid, &IID_IUnknown) || IsEqualGUID(iid, &IID_IReferenceClock))
@@ -127,7 +128,7 @@ static HRESULT WINAPI SystemClockImpl_QueryInterface(IReferenceClock *iface, REF

static ULONG WINAPI SystemClockImpl_AddRef(IReferenceClock *iface)
 {
-    SystemClockImpl *clock = impl_from_IReferenceClock(iface);
+    struct system_clock *clock = impl_from_IReferenceClock(iface);
     ULONG refcount = InterlockedIncrement(&clock->ref);

TRACE("%p increasing refcount to %u.\n", clock, refcount);
@@ -137,7 +138,7 @@ static ULONG WINAPI SystemClockImpl_AddRef(IReferenceClock *iface)

static ULONG WINAPI SystemClockImpl_Release(IReferenceClock *iface)
 {
-    SystemClockImpl *clock = impl_from_IReferenceClock(iface);
+    struct system_clock *clock = impl_from_IReferenceClock(iface);
     ULONG refcount = InterlockedDecrement(&clock->ref);

TRACE("%p decreasing refcount to %u.\n", clock, refcount);
@@ -161,7 +162,7 @@ static ULONG WINAPI SystemClockImpl_Release(IReferenceClock *iface)

static HRESULT WINAPI SystemClockImpl_GetTime(IReferenceClock *iface, REFERENCE_TIME *time)
 {
-    SystemClockImpl *clock = impl_from_IReferenceClock(iface);
+    struct system_clock *clock = impl_from_IReferenceClock(iface);
     REFERENCE_TIME ret;
     HRESULT hr;

@@ -186,7 +187,7 @@ static HRESULT WINAPI SystemClockImpl_GetTime(IReferenceClock *iface, REFERENCE_
 static HRESULT WINAPI SystemClockImpl_AdviseTime(IReferenceClock *iface,
         REFERENCE_TIME base, REFERENCE_TIME offset, HEVENT event, DWORD_PTR *cookie)
 {
-    SystemClockImpl *clock = impl_from_IReferenceClock(iface);
+    struct system_clock *clock = impl_from_IReferenceClock(iface);
     struct advise_sink *sink;

TRACE("clock %p, base %s, offset %s, event %#lx, cookie %p.\n",
@@ -222,7 +223,7 @@ static HRESULT WINAPI SystemClockImpl_AdviseTime(IReferenceClock *iface,
 static HRESULT WINAPI SystemClockImpl_AdvisePeriodic(IReferenceClock* iface,
         REFERENCE_TIME start, REFERENCE_TIME period, HSEMAPHORE semaphore, DWORD_PTR *cookie)
 {
-    SystemClockImpl *clock = impl_from_IReferenceClock(iface);
+    struct system_clock *clock = impl_from_IReferenceClock(iface);
     struct advise_sink *sink;

TRACE("clock %p, start %s, period %s, semaphore %#lx, cookie %p.\n",
@@ -257,7 +258,7 @@ static HRESULT WINAPI SystemClockImpl_AdvisePeriodic(IReferenceClock* iface,

static HRESULT WINAPI SystemClockImpl_Unadvise(IReferenceClock *iface, DWORD_PTR cookie)
 {
-    SystemClockImpl *clock = impl_from_IReferenceClock(iface);
+    struct system_clock *clock = impl_from_IReferenceClock(iface);
     struct advise_sink *sink;

TRACE("clock %p, cookie %#lx.\n", clock, cookie);
@@ -293,7 +294,7 @@ static const IReferenceClockVtbl SystemClock_Vtbl =

HRESULT QUARTZ_CreateSystemClock(IUnknown *outer, void **out)
 {
-    SystemClockImpl *object;
+    struct system_clock *object;

TRACE("outer %p, out %p.\n", outer, out);

[Marvin]

Hi,

While running your changed tests, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?

Full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=49487

Your paranoid android.

=== debian9b (64 bit WoW report) ===

quartz:
Unhandled exception: page fault on read access to 0x00000000 in 64-bit code (0x00007f282fc9a00c).

Report errors:
quartz:avisplit crashed (c0000005)