git.sesse.net Git - nageru/blob - patches/mesa-fix-locking-of-glsync-objects.diff

   1 From 6e3d1880fa78a3a965cb7eb51ee12b1f785f84bb Mon Sep 17 00:00:00 2001
   2 From: "Steinar H. Gunderson" <sesse@google.com>
   3 Date: Tue, 1 Dec 2015 22:05:11 +0100
   4 Subject: [PATCH] Fix locking of GLsync objects.
   5
   6 GLsync objects had a race condition when used from multiple threads
   7 (which is the main point of the extension, really); it could be
   8 validated as a sync object at the beginning of the function, and then
   9 deleted by another thread before use, causing crashes. Fix this by
  10 changing all casts from GLsync to struct gl_sync_object to a new
  11 function _mesa_get_sync() that validates and increases the refcount.
  12
  13 In a similar vein, validation itself uses _mesa_set_search(), which
  14 requires synchronization -- it was called without a mutex held, causing
  15 spurious error returns and other issues. Since _mesa_get_sync() now
  16 takes the shared context mutex, this problem is also resolved.
  17
  18 Signed-off-by: Steinar H. Gunderson <sesse@google.com>
  19 ---
  20  src/mesa/main/objectlabel.c | 11 ++++--
  21  src/mesa/main/shared.c      |  2 +-
  22  src/mesa/main/syncobj.c     | 89 ++++++++++++++++++++++++++-------------------
  23  src/mesa/main/syncobj.h     | 11 ++----
  24  4 files changed, 64 insertions(+), 49 deletions(-)
  25
  26 diff --git a/src/mesa/main/objectlabel.c b/src/mesa/main/objectlabel.c
  27 index 41f370c..b083c43 100644
  28 --- a/src/mesa/main/objectlabel.c
  29 +++ b/src/mesa/main/objectlabel.c
  30 @@ -288,7 +288,7 @@ void GLAPIENTRY
  31  _mesa_ObjectPtrLabel(const void *ptr, GLsizei length, const GLchar *label)
  32  {
  33     GET_CURRENT_CONTEXT(ctx);
  34 -   struct gl_sync_object *const syncObj = (struct gl_sync_object *) ptr;
  35 +   struct gl_sync_object *syncObj = _mesa_get_sync(ctx, sync, true);
  36     const char *callerstr;
  37     char **labelPtr;
  38
  39 @@ -297,7 +297,7 @@ _mesa_ObjectPtrLabel(const void *ptr, GLsizei length, const GLchar *label)
  40     else
  41        callerstr = "glObjectPtrLabelKHR";
  42
  43 -   if (!_mesa_validate_sync(ctx, syncObj)) {
  44 +   if (!syncObj) {
  45        _mesa_error(ctx, GL_INVALID_VALUE, "%s (not a valid sync object)",
  46                    callerstr);
  47        return;
  48 @@ -306,6 +306,7 @@ _mesa_ObjectPtrLabel(const void *ptr, GLsizei length, const GLchar *label)
  49     labelPtr = &syncObj->Label;
  50
  51     set_label(ctx, labelPtr, label, length, callerstr);
  52 +   _mesa_unref_sync_object(ctx, syncObj, 1);
  53  }
  54
  55  void GLAPIENTRY
  56 @@ -313,7 +314,7 @@ _mesa_GetObjectPtrLabel(const void *ptr, GLsizei bufSize, GLsizei *length,
  57                          GLchar *label)
  58  {
  59     GET_CURRENT_CONTEXT(ctx);
  60 -   struct gl_sync_object *const syncObj = (struct gl_sync_object *) ptr;
  61 +   struct gl_sync_object *syncObj;
  62     const char *callerstr;
  63     char **labelPtr;
  64
  65 @@ -328,7 +329,8 @@ _mesa_GetObjectPtrLabel(const void *ptr, GLsizei bufSize, GLsizei *length,
  66        return;
  67     }
  68
  69 -   if (!_mesa_validate_sync(ctx, syncObj)) {
  70 +   syncObj = _mesa_get_sync(ctx, sync, true);
  71 +   if (!syncObj) {
  72        _mesa_error(ctx, GL_INVALID_VALUE, "%s (not a valid sync object)",
  73                    callerstr);
  74        return;
  75 @@ -337,4 +339,5 @@ _mesa_GetObjectPtrLabel(const void *ptr, GLsizei bufSize, GLsizei *length,
  76     labelPtr = &syncObj->Label;
  77
  78     copy_label(*labelPtr, label, length, bufSize);
  79 +   _mesa_unref_sync_object(ctx, syncObj, 1);
  80  }
  81 diff --git a/src/mesa/main/shared.c b/src/mesa/main/shared.c
  82 index c37b31d..b9f7bb6 100644
  83 --- a/src/mesa/main/shared.c
  84 +++ b/src/mesa/main/shared.c
  85 @@ -338,7 +338,7 @@ free_shared_state(struct gl_context *ctx, struct gl_shared_state *shared)
  86        struct set_entry *entry;
  87
  88        set_foreach(shared->SyncObjects, entry) {
  89 -         _mesa_unref_sync_object(ctx, (struct gl_sync_object *) entry->key);
  90 +         _mesa_unref_sync_object(ctx, (struct gl_sync_object *) entry->key, 1);
  91        }
  92     }
  93     _mesa_set_destroy(shared->SyncObjects, NULL);
  94 diff --git a/src/mesa/main/syncobj.c b/src/mesa/main/syncobj.c
  95 index c1b2d3b..d1c6c06 100644
  96 --- a/src/mesa/main/syncobj.c
  97 +++ b/src/mesa/main/syncobj.c
  98 @@ -167,34 +167,42 @@ _mesa_free_sync_data(struct gl_context *ctx)
  99   *  - not in sync objects hash table
 100   *  - type is GL_SYNC_FENCE
 101   *  - not marked as deleted
 102 + *
 103 + * Returns the internal gl_sync_object pointer if the sync object is valid
 104 + * or NULL if it isn't.
 105 + *
 106 + * If "incRefCount" is true, the reference count is incremented, which is
 107 + * normally what you want; otherwise, a glDeleteSync from another thread
 108 + * could delete the sync object while you are still working on it.
 109   */
 110 -bool
 111 -_mesa_validate_sync(struct gl_context *ctx,
 112 -                    const struct gl_sync_object *syncObj)
 113 +struct gl_sync_object *
 114 +_mesa_get_sync(struct gl_context *ctx, GLsync sync, bool incRefCount)
 115  {
 116 -   return (syncObj != NULL)
 117 +   struct gl_sync_object *syncObj = (struct gl_sync_object *) sync;
 118 +   mtx_lock(&ctx->Shared->Mutex);
 119 +   if (syncObj != NULL
 120        && _mesa_set_search(ctx->Shared->SyncObjects, syncObj) != NULL
 121        && (syncObj->Type == GL_SYNC_FENCE)
 122 -      && !syncObj->DeletePending;
 123 -}
 124 -
 125 -
 126 -void
 127 -_mesa_ref_sync_object(struct gl_context *ctx, struct gl_sync_object *syncObj)
 128 -{
 129 -   mtx_lock(&ctx->Shared->Mutex);
 130 -   syncObj->RefCount++;
 131 +      && !syncObj->DeletePending) {
 132 +     if (incRefCount) {
 133 +       syncObj->RefCount++;
 134 +     }
 135 +   } else {
 136 +     syncObj = NULL;
 137 +   }
 138     mtx_unlock(&ctx->Shared->Mutex);
 139 +   return syncObj;
 140  }
 141
 142
 143  void
 144 -_mesa_unref_sync_object(struct gl_context *ctx, struct gl_sync_object *syncObj)
 145 +_mesa_unref_sync_object(struct gl_context *ctx, struct gl_sync_object *syncObj,
 146 +                        int amount)
 147  {
 148     struct set_entry *entry;
 149
 150     mtx_lock(&ctx->Shared->Mutex);
 151 -   syncObj->RefCount--;
 152 +   syncObj->RefCount -= amount;
 153     if (syncObj->RefCount == 0) {
 154        entry = _mesa_set_search(ctx->Shared->SyncObjects, syncObj);
 155        assert (entry != NULL);
 156 @@ -212,10 +220,9 @@ GLboolean GLAPIENTRY
 157  _mesa_IsSync(GLsync sync)
 158  {
 159     GET_CURRENT_CONTEXT(ctx);
 160 -   struct gl_sync_object *const syncObj = (struct gl_sync_object *) sync;
 161     ASSERT_OUTSIDE_BEGIN_END_WITH_RETVAL(ctx, GL_FALSE);
 162
 163 -   return _mesa_validate_sync(ctx, syncObj) ? GL_TRUE : GL_FALSE;
 164 +   return _mesa_get_sync(ctx, sync, false) ? GL_TRUE : GL_FALSE;
 165  }
 166
 167
 168 @@ -223,7 +230,7 @@ void GLAPIENTRY
 169  _mesa_DeleteSync(GLsync sync)
 170  {
 171     GET_CURRENT_CONTEXT(ctx);
 172 -   struct gl_sync_object *const syncObj = (struct gl_sync_object *) sync;
 173 +   struct gl_sync_object *syncObj;
 174
 175     /* From the GL_ARB_sync spec:
 176      *
 177 @@ -235,16 +242,19 @@ _mesa_DeleteSync(GLsync sync)
 178        return;
 179     }
 180
 181 -   if (!_mesa_validate_sync(ctx, syncObj)) {
 182 +   syncObj = _mesa_get_sync(ctx, sync, true);
 183 +   if (!syncObj) {
 184        _mesa_error(ctx, GL_INVALID_VALUE, "glDeleteSync (not a valid sync object)");
 185        return;
 186     }
 187
 188     /* If there are no client-waits or server-waits pending on this sync, delete
 189 -    * the underlying object.
 190 +    * the underlying object. Note that we double-unref the object, as _mesa_get_sync
 191 +    * above took an extra refcount to make sure the pointer is valid for us to
 192 +    * manipulate.
 193      */
 194     syncObj->DeletePending = GL_TRUE;
 195 -   _mesa_unref_sync_object(ctx, syncObj);
 196 +   _mesa_unref_sync_object(ctx, syncObj, 2);
 197  }
 198
 199
 200 @@ -299,21 +309,20 @@ GLenum GLAPIENTRY
 201  _mesa_ClientWaitSync(GLsync sync, GLbitfield flags, GLuint64 timeout)
 202  {
 203     GET_CURRENT_CONTEXT(ctx);
 204 -   struct gl_sync_object *const syncObj = (struct gl_sync_object *) sync;
 205 +   struct gl_sync_object *syncObj;
 206     GLenum ret;
 207     ASSERT_OUTSIDE_BEGIN_END_WITH_RETVAL(ctx, GL_WAIT_FAILED);
 208
 209 -   if (!_mesa_validate_sync(ctx, syncObj)) {
 210 -      _mesa_error(ctx, GL_INVALID_VALUE, "glClientWaitSync (not a valid sync object)");
 211 -      return GL_WAIT_FAILED;
 212 -   }
 213 -
 214     if ((flags & ~GL_SYNC_FLUSH_COMMANDS_BIT) != 0) {
 215        _mesa_error(ctx, GL_INVALID_VALUE, "glClientWaitSync(flags=0x%x)", flags);
 216        return GL_WAIT_FAILED;
 217     }
 218
 219 -   _mesa_ref_sync_object(ctx, syncObj);
 220 +   syncObj = _mesa_get_sync(ctx, sync, true);
 221 +   if (!syncObj) {
 222 +      _mesa_error(ctx, GL_INVALID_VALUE, "glClientWaitSync (not a valid sync object)");
 223 +      return GL_WAIT_FAILED;
 224 +   }
 225
 226     /* From the GL_ARB_sync spec:
 227      *
 228 @@ -335,7 +344,7 @@ _mesa_ClientWaitSync(GLsync sync, GLbitfield flags, GLuint64 timeout)
 229        }
 230     }
 231
 232 -   _mesa_unref_sync_object(ctx, syncObj);
 233 +   _mesa_unref_sync_object(ctx, syncObj, 1);
 234     return ret;
 235  }
 236
 237 @@ -344,12 +353,7 @@ void GLAPIENTRY
 238  _mesa_WaitSync(GLsync sync, GLbitfield flags, GLuint64 timeout)
 239  {
 240     GET_CURRENT_CONTEXT(ctx);
 241 -   struct gl_sync_object *const syncObj = (struct gl_sync_object *) sync;
 242 -
 243 -   if (!_mesa_validate_sync(ctx, syncObj)) {
 244 -      _mesa_error(ctx, GL_INVALID_VALUE, "glWaitSync (not a valid sync object)");
 245 -      return;
 246 -   }
 247 +   struct gl_sync_object *syncObj;
 248
 249     if (flags != 0) {
 250        _mesa_error(ctx, GL_INVALID_VALUE, "glWaitSync(flags=0x%x)", flags);
 251 @@ -362,7 +366,14 @@ _mesa_WaitSync(GLsync sync, GLbitfield flags, GLuint64 timeout)
 252        return;
 253     }
 254
 255 +   syncObj = _mesa_get_sync(ctx, sync, true);
 256 +   if (!syncObj) {
 257 +      _mesa_error(ctx, GL_INVALID_VALUE, "glWaitSync (not a valid sync object)");
 258 +      return;
 259 +   }
 260 +
 261     ctx->Driver.ServerWaitSync(ctx, syncObj, flags, timeout);
 262 +   _mesa_unref_sync_object(ctx, syncObj, 1);
 263  }
 264
 265
 266 @@ -371,11 +382,12 @@ _mesa_GetSynciv(GLsync sync, GLenum pname, GLsizei bufSize, GLsizei *length,
 267                 GLint *values)
 268  {
 269     GET_CURRENT_CONTEXT(ctx);
 270 -   struct gl_sync_object *const syncObj = (struct gl_sync_object *) sync;
 271 +   struct gl_sync_object *syncObj;
 272     GLsizei size = 0;
 273     GLint v[1];
 274
 275 -   if (!_mesa_validate_sync(ctx, syncObj)) {
 276 +   syncObj = _mesa_get_sync(ctx, sync, true);
 277 +   if (!syncObj) {
 278        _mesa_error(ctx, GL_INVALID_VALUE, "glGetSynciv (not a valid sync object)");
 279        return;
 280     }
 281 @@ -409,6 +421,7 @@ _mesa_GetSynciv(GLsync sync, GLenum pname, GLsizei bufSize, GLsizei *length,
 282
 283     default:
 284        _mesa_error(ctx, GL_INVALID_ENUM, "glGetSynciv(pname=0x%x)\n", pname);
 285 +      _mesa_unref_sync_object(ctx, syncObj, 1);
 286        return;
 287     }
 288
 289 @@ -421,4 +434,6 @@ _mesa_GetSynciv(GLsync sync, GLenum pname, GLsizei bufSize, GLsizei *length,
 290     if (length != NULL) {
 291        *length = size;
 292     }
 293 +
 294 +   _mesa_unref_sync_object(ctx, syncObj, 1);
 295  }
 296 diff --git a/src/mesa/main/syncobj.h b/src/mesa/main/syncobj.h
 297 index 5d510e8..e8dbded 100644
 298 --- a/src/mesa/main/syncobj.h
 299 +++ b/src/mesa/main/syncobj.h
 300 @@ -47,15 +47,12 @@ _mesa_init_sync(struct gl_context *);
 301  extern void
 302  _mesa_free_sync_data(struct gl_context *);
 303
 304 -extern void
 305 -_mesa_ref_sync_object(struct gl_context *ctx, struct gl_sync_object *syncObj);
 306 +struct gl_sync_object *
 307 +_mesa_get_sync(struct gl_context *ctx, GLsync sync, bool incRefCount);
 308
 309  extern void
 310 -_mesa_unref_sync_object(struct gl_context *ctx, struct gl_sync_object *syncObj);
 311 -
 312 -extern bool
 313 -_mesa_validate_sync(struct gl_context *ctx,
 314 -                    const struct gl_sync_object *syncObj);
 315 +_mesa_unref_sync_object(struct gl_context *ctx, struct gl_sync_object *syncObj,
 316 +                        int amount);
 317
 318  extern GLboolean GLAPIENTRY
 319  _mesa_IsSync(GLsync sync);
 320 --
 321 2.6.2