mesa: Fix locking of GLsync objects.

GLsync objects had a race condition when used from multiple threads
(which is the main point of the extension, really); it could be
validated as a sync object at the beginning of the function, and then
deleted by another thread before use, causing crashes. Fix this by
changing all casts from GLsync to struct gl_sync_object to a new
function _mesa_get_and_ref_sync() that validates and increases
the refcount.

In a similar vein, validation itself uses _mesa_set_search(), which
requires synchronization -- it was called without a mutex held, causing
spurious error returns and other issues. Since _mesa_get_and_ref_sync()
now takes the shared context mutex, this problem is also resolved.

Fixes bug #92757, found while developing Nageru, my live video mixer
(due for release at FOSDEM 2016).

v2: Marek: silence warnings, fix declaration after code

Signed-off-by: Steinar H. Gunderson <sesse@google.com>
Cc: "11.0 11.1" <mesa-stable@lists.freedesktop.org>
Signed-off-by: Marek Olšák <marek.olsak@amd.com>

diff --git a/src/mesa/main/objectlabel.c b/src/mesa/main/objectlabel.c
index 41f370ce48519219e88bc0b15a229a82a774ecf0..b622d6a2979227350da5f8f5a92e735d4f26844e 100644 (file)
--- a/src/mesa/main/objectlabel.c
+++ b/src/mesa/main/objectlabel.c
@@ -288,16 +288,18 @@ void GLAPIENTRY
 _mesa_ObjectPtrLabel(const void *ptr, GLsizei length, const GLchar *label)
 {
    GET_CURRENT_CONTEXT(ctx);
-   struct gl_sync_object *const syncObj = (struct gl_sync_object *) ptr;
+   struct gl_sync_object *syncObj;
    const char *callerstr;
    char **labelPtr;
 
+   syncObj = _mesa_get_and_ref_sync(ctx, (void*)ptr, true);
+
    if (_mesa_is_desktop_gl(ctx))
       callerstr = "glObjectPtrLabel";
    else
       callerstr = "glObjectPtrLabelKHR";
 
-   if (!_mesa_validate_sync(ctx, syncObj)) {
+   if (!syncObj) {
       _mesa_error(ctx, GL_INVALID_VALUE, "%s (not a valid sync object)",
                   callerstr);
       return;
@@ -306,6 +308,7 @@ _mesa_ObjectPtrLabel(const void *ptr, GLsizei length, const GLchar *label)
    labelPtr = &syncObj->Label;
 
    set_label(ctx, labelPtr, label, length, callerstr);
+   _mesa_unref_sync_object(ctx, syncObj, 1);
 }
 
 void GLAPIENTRY
@@ -313,7 +316,7 @@ _mesa_GetObjectPtrLabel(const void *ptr, GLsizei bufSize, GLsizei *length,
                         GLchar *label)
 {
    GET_CURRENT_CONTEXT(ctx);
-   struct gl_sync_object *const syncObj = (struct gl_sync_object *) ptr;
+   struct gl_sync_object *syncObj;
    const char *callerstr;
    char **labelPtr;
 
@@ -328,7 +331,8 @@ _mesa_GetObjectPtrLabel(const void *ptr, GLsizei bufSize, GLsizei *length,
       return;
    }
 
-   if (!_mesa_validate_sync(ctx, syncObj)) {
+   syncObj = _mesa_get_and_ref_sync(ctx, (void*)ptr, true);
+   if (!syncObj) {
       _mesa_error(ctx, GL_INVALID_VALUE, "%s (not a valid sync object)",
                   callerstr);
       return;
@@ -337,4 +341,5 @@ _mesa_GetObjectPtrLabel(const void *ptr, GLsizei bufSize, GLsizei *length,
    labelPtr = &syncObj->Label;
 
    copy_label(*labelPtr, label, length, bufSize);
+   _mesa_unref_sync_object(ctx, syncObj, 1);
 }

diff --git a/src/mesa/main/shared.c b/src/mesa/main/shared.c
index c37b31d1753e45455627382d936c18febceaa2d4..b9f7bb65fb66201eaec5506fd70217be761082e0 100644 (file)
--- a/src/mesa/main/shared.c
+++ b/src/mesa/main/shared.c
@@ -338,7 +338,7 @@ free_shared_state(struct gl_context *ctx, struct gl_shared_state *shared)
       struct set_entry *entry;
 
       set_foreach(shared->SyncObjects, entry) {
-         _mesa_unref_sync_object(ctx, (struct gl_sync_object *) entry->key);
+         _mesa_unref_sync_object(ctx, (struct gl_sync_object *) entry->key, 1);
       }
    }
    _mesa_set_destroy(shared->SyncObjects, NULL);

diff --git a/src/mesa/main/syncobj.c b/src/mesa/main/syncobj.c
index c1b2d3bed54d1a25b6fb30659a240af0498cab96..be758dd1241f03fddf2f199a245cc8d2b844d5bc 100644 (file)
--- a/src/mesa/main/syncobj.c
+++ b/src/mesa/main/syncobj.c
@@ -167,34 +167,42 @@ _mesa_free_sync_data(struct gl_context *ctx)
  *  - not in sync objects hash table
  *  - type is GL_SYNC_FENCE
  *  - not marked as deleted
+ *
+ * Returns the internal gl_sync_object pointer if the sync object is valid
+ * or NULL if it isn't.
+ *
+ * If "incRefCount" is true, the reference count is incremented, which is
+ * normally what you want; otherwise, a glDeleteSync from another thread
+ * could delete the sync object while you are still working on it.
  */
-bool
-_mesa_validate_sync(struct gl_context *ctx,
-                    const struct gl_sync_object *syncObj)
+struct gl_sync_object *
+_mesa_get_and_ref_sync(struct gl_context *ctx, GLsync sync, bool incRefCount)
 {
-   return (syncObj != NULL)
+   struct gl_sync_object *syncObj = (struct gl_sync_object *) sync;
+   mtx_lock(&ctx->Shared->Mutex);
+   if (syncObj != NULL
       && _mesa_set_search(ctx->Shared->SyncObjects, syncObj) != NULL
       && (syncObj->Type == GL_SYNC_FENCE)
-      && !syncObj->DeletePending;
-}
-
-
-void
-_mesa_ref_sync_object(struct gl_context *ctx, struct gl_sync_object *syncObj)
-{
-   mtx_lock(&ctx->Shared->Mutex);
-   syncObj->RefCount++;
+      && !syncObj->DeletePending) {
+     if (incRefCount) {
+       syncObj->RefCount++;
+     }
+   } else {
+     syncObj = NULL;
+   }
    mtx_unlock(&ctx->Shared->Mutex);
+   return syncObj;
 }
 
 
 void
-_mesa_unref_sync_object(struct gl_context *ctx, struct gl_sync_object *syncObj)
+_mesa_unref_sync_object(struct gl_context *ctx, struct gl_sync_object *syncObj,
+                        int amount)
 {
    struct set_entry *entry;
 
    mtx_lock(&ctx->Shared->Mutex);
-   syncObj->RefCount--;
+   syncObj->RefCount -= amount;
    if (syncObj->RefCount == 0) {
       entry = _mesa_set_search(ctx->Shared->SyncObjects, syncObj);
       assert (entry != NULL);
@@ -212,10 +220,9 @@ GLboolean GLAPIENTRY
 _mesa_IsSync(GLsync sync)
 {
    GET_CURRENT_CONTEXT(ctx);
-   struct gl_sync_object *const syncObj = (struct gl_sync_object *) sync;
    ASSERT_OUTSIDE_BEGIN_END_WITH_RETVAL(ctx, GL_FALSE);
 
-   return _mesa_validate_sync(ctx, syncObj) ? GL_TRUE : GL_FALSE;
+   return _mesa_get_and_ref_sync(ctx, sync, false) ? GL_TRUE : GL_FALSE;
 }
 
 
@@ -223,7 +230,7 @@ void GLAPIENTRY
 _mesa_DeleteSync(GLsync sync)
 {
    GET_CURRENT_CONTEXT(ctx);
-   struct gl_sync_object *const syncObj = (struct gl_sync_object *) sync;
+   struct gl_sync_object *syncObj;
 
    /* From the GL_ARB_sync spec:
     *
@@ -235,16 +242,19 @@ _mesa_DeleteSync(GLsync sync)
       return;
    }
 
-   if (!_mesa_validate_sync(ctx, syncObj)) {
+   syncObj = _mesa_get_and_ref_sync(ctx, sync, true);
+   if (!syncObj) {
       _mesa_error(ctx, GL_INVALID_VALUE, "glDeleteSync (not a valid sync object)");
       return;
    }
 
    /* If there are no client-waits or server-waits pending on this sync, delete
-    * the underlying object.
+    * the underlying object. Note that we double-unref the object, as
+    * _mesa_get_and_ref_sync above took an extra refcount to make sure the pointer
+    * is valid for us to manipulate.
     */
    syncObj->DeletePending = GL_TRUE;
-   _mesa_unref_sync_object(ctx, syncObj);
+   _mesa_unref_sync_object(ctx, syncObj, 2);
 }
 
 
@@ -299,21 +309,20 @@ GLenum GLAPIENTRY
 _mesa_ClientWaitSync(GLsync sync, GLbitfield flags, GLuint64 timeout)
 {
    GET_CURRENT_CONTEXT(ctx);
-   struct gl_sync_object *const syncObj = (struct gl_sync_object *) sync;
+   struct gl_sync_object *syncObj;
    GLenum ret;
    ASSERT_OUTSIDE_BEGIN_END_WITH_RETVAL(ctx, GL_WAIT_FAILED);
 
-   if (!_mesa_validate_sync(ctx, syncObj)) {
-      _mesa_error(ctx, GL_INVALID_VALUE, "glClientWaitSync (not a valid sync object)");
-      return GL_WAIT_FAILED;
-   }
-
    if ((flags & ~GL_SYNC_FLUSH_COMMANDS_BIT) != 0) {
       _mesa_error(ctx, GL_INVALID_VALUE, "glClientWaitSync(flags=0x%x)", flags);
       return GL_WAIT_FAILED;
    }
 
-   _mesa_ref_sync_object(ctx, syncObj);
+   syncObj = _mesa_get_and_ref_sync(ctx, sync, true);
+   if (!syncObj) {
+      _mesa_error(ctx, GL_INVALID_VALUE, "glClientWaitSync (not a valid sync object)");
+      return GL_WAIT_FAILED;
+   }
 
    /* From the GL_ARB_sync spec:
     *
@@ -335,7 +344,7 @@ _mesa_ClientWaitSync(GLsync sync, GLbitfield flags, GLuint64 timeout)
       }
    }
 
-   _mesa_unref_sync_object(ctx, syncObj);
+   _mesa_unref_sync_object(ctx, syncObj, 1);
    return ret;
 }
 
@@ -344,12 +353,7 @@ void GLAPIENTRY
 _mesa_WaitSync(GLsync sync, GLbitfield flags, GLuint64 timeout)
 {
    GET_CURRENT_CONTEXT(ctx);
-   struct gl_sync_object *const syncObj = (struct gl_sync_object *) sync;
-
-   if (!_mesa_validate_sync(ctx, syncObj)) {
-      _mesa_error(ctx, GL_INVALID_VALUE, "glWaitSync (not a valid sync object)");
-      return;
-   }
+   struct gl_sync_object *syncObj;
 
    if (flags != 0) {
       _mesa_error(ctx, GL_INVALID_VALUE, "glWaitSync(flags=0x%x)", flags);
@@ -362,7 +366,14 @@ _mesa_WaitSync(GLsync sync, GLbitfield flags, GLuint64 timeout)
       return;
    }
 
+   syncObj = _mesa_get_and_ref_sync(ctx, sync, true);
+   if (!syncObj) {
+      _mesa_error(ctx, GL_INVALID_VALUE, "glWaitSync (not a valid sync object)");
+      return;
+   }
+
    ctx->Driver.ServerWaitSync(ctx, syncObj, flags, timeout);
+   _mesa_unref_sync_object(ctx, syncObj, 1);
 }
 
 
@@ -371,11 +382,12 @@ _mesa_GetSynciv(GLsync sync, GLenum pname, GLsizei bufSize, GLsizei *length,
                GLint *values)
 {
    GET_CURRENT_CONTEXT(ctx);
-   struct gl_sync_object *const syncObj = (struct gl_sync_object *) sync;
+   struct gl_sync_object *syncObj;
    GLsizei size = 0;
    GLint v[1];
 
-   if (!_mesa_validate_sync(ctx, syncObj)) {
+   syncObj = _mesa_get_and_ref_sync(ctx, sync, true);
+   if (!syncObj) {
       _mesa_error(ctx, GL_INVALID_VALUE, "glGetSynciv (not a valid sync object)");
       return;
    }
@@ -409,6 +421,7 @@ _mesa_GetSynciv(GLsync sync, GLenum pname, GLsizei bufSize, GLsizei *length,
 
    default:
       _mesa_error(ctx, GL_INVALID_ENUM, "glGetSynciv(pname=0x%x)\n", pname);
+      _mesa_unref_sync_object(ctx, syncObj, 1);
       return;
    }
 
@@ -421,4 +434,6 @@ _mesa_GetSynciv(GLsync sync, GLenum pname, GLsizei bufSize, GLsizei *length,
    if (length != NULL) {
       *length = size;
    }
+
+   _mesa_unref_sync_object(ctx, syncObj, 1);
 }

diff --git a/src/mesa/main/syncobj.h b/src/mesa/main/syncobj.h
index 5d510e873a9224b8ad434ed6fd46e137041b6d70..ea4a71222c0b726e617219946212fc188181a747 100644 (file)
--- a/src/mesa/main/syncobj.h
+++ b/src/mesa/main/syncobj.h
@@ -47,15 +47,12 @@ _mesa_init_sync(struct gl_context *);
 extern void
 _mesa_free_sync_data(struct gl_context *);
 
-extern void
-_mesa_ref_sync_object(struct gl_context *ctx, struct gl_sync_object *syncObj);
+struct gl_sync_object *
+_mesa_get_and_ref_sync(struct gl_context *ctx, GLsync sync, bool incRefCount);
 
 extern void
-_mesa_unref_sync_object(struct gl_context *ctx, struct gl_sync_object *syncObj);
-
-extern bool
-_mesa_validate_sync(struct gl_context *ctx,
-                    const struct gl_sync_object *syncObj);
+_mesa_unref_sync_object(struct gl_context *ctx, struct gl_sync_object *syncObj,
+                        int amount);
 
 extern GLboolean GLAPIENTRY
 _mesa_IsSync(GLsync sync);