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.
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.
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.
18 Signed-off-by: Steinar H. Gunderson <sesse@google.com>
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(-)
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)
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;
39 @@ -297,7 +297,7 @@ _mesa_ObjectPtrLabel(const void *ptr, GLsizei length, const GLchar *label)
41 callerstr = "glObjectPtrLabelKHR";
43 - if (!_mesa_validate_sync(ctx, syncObj)) {
45 _mesa_error(ctx, GL_INVALID_VALUE, "%s (not a valid sync object)",
48 @@ -306,6 +306,7 @@ _mesa_ObjectPtrLabel(const void *ptr, GLsizei length, const GLchar *label)
49 labelPtr = &syncObj->Label;
51 set_label(ctx, labelPtr, label, length, callerstr);
52 + _mesa_unref_sync_object(ctx, syncObj, 1);
56 @@ -313,7 +314,7 @@ _mesa_GetObjectPtrLabel(const void *ptr, GLsizei bufSize, GLsizei *length,
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;
65 @@ -328,7 +329,8 @@ _mesa_GetObjectPtrLabel(const void *ptr, GLsizei bufSize, GLsizei *length,
69 - if (!_mesa_validate_sync(ctx, syncObj)) {
70 + syncObj = _mesa_get_sync(ctx, sync, true);
72 _mesa_error(ctx, GL_INVALID_VALUE, "%s (not a valid sync object)",
75 @@ -337,4 +339,5 @@ _mesa_GetObjectPtrLabel(const void *ptr, GLsizei bufSize, GLsizei *length,
76 labelPtr = &syncObj->Label;
78 copy_label(*labelPtr, label, length, bufSize);
79 + _mesa_unref_sync_object(ctx, syncObj, 1);
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;
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);
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
103 + * Returns the internal gl_sync_object pointer if the sync object is valid
104 + * or NULL if it isn't.
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.
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)
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;
127 -_mesa_ref_sync_object(struct gl_context *ctx, struct gl_sync_object *syncObj)
129 - mtx_lock(&ctx->Shared->Mutex);
130 - syncObj->RefCount++;
131 + && !syncObj->DeletePending) {
133 + syncObj->RefCount++;
138 mtx_unlock(&ctx->Shared->Mutex);
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,
148 struct set_entry *entry;
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)
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);
163 - return _mesa_validate_sync(ctx, syncObj) ? GL_TRUE : GL_FALSE;
164 + return _mesa_get_sync(ctx, sync, false) ? GL_TRUE : GL_FALSE;
168 @@ -223,7 +230,7 @@ void GLAPIENTRY
169 _mesa_DeleteSync(GLsync sync)
171 GET_CURRENT_CONTEXT(ctx);
172 - struct gl_sync_object *const syncObj = (struct gl_sync_object *) sync;
173 + struct gl_sync_object *syncObj;
175 /* From the GL_ARB_sync spec:
177 @@ -235,16 +242,19 @@ _mesa_DeleteSync(GLsync sync)
181 - if (!_mesa_validate_sync(ctx, syncObj)) {
182 + syncObj = _mesa_get_sync(ctx, sync, true);
184 _mesa_error(ctx, GL_INVALID_VALUE, "glDeleteSync (not a valid sync object)");
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
194 syncObj->DeletePending = GL_TRUE;
195 - _mesa_unref_sync_object(ctx, syncObj);
196 + _mesa_unref_sync_object(ctx, syncObj, 2);
200 @@ -299,21 +309,20 @@ GLenum GLAPIENTRY
201 _mesa_ClientWaitSync(GLsync sync, GLbitfield flags, GLuint64 timeout)
203 GET_CURRENT_CONTEXT(ctx);
204 - struct gl_sync_object *const syncObj = (struct gl_sync_object *) sync;
205 + struct gl_sync_object *syncObj;
207 ASSERT_OUTSIDE_BEGIN_END_WITH_RETVAL(ctx, GL_WAIT_FAILED);
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;
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;
219 - _mesa_ref_sync_object(ctx, syncObj);
220 + syncObj = _mesa_get_sync(ctx, sync, true);
222 + _mesa_error(ctx, GL_INVALID_VALUE, "glClientWaitSync (not a valid sync object)");
223 + return GL_WAIT_FAILED;
226 /* From the GL_ARB_sync spec:
228 @@ -335,7 +344,7 @@ _mesa_ClientWaitSync(GLsync sync, GLbitfield flags, GLuint64 timeout)
232 - _mesa_unref_sync_object(ctx, syncObj);
233 + _mesa_unref_sync_object(ctx, syncObj, 1);
237 @@ -344,12 +353,7 @@ void GLAPIENTRY
238 _mesa_WaitSync(GLsync sync, GLbitfield flags, GLuint64 timeout)
240 GET_CURRENT_CONTEXT(ctx);
241 - struct gl_sync_object *const syncObj = (struct gl_sync_object *) sync;
243 - if (!_mesa_validate_sync(ctx, syncObj)) {
244 - _mesa_error(ctx, GL_INVALID_VALUE, "glWaitSync (not a valid sync object)");
247 + struct gl_sync_object *syncObj;
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)
255 + syncObj = _mesa_get_sync(ctx, sync, true);
257 + _mesa_error(ctx, GL_INVALID_VALUE, "glWaitSync (not a valid sync object)");
261 ctx->Driver.ServerWaitSync(ctx, syncObj, flags, timeout);
262 + _mesa_unref_sync_object(ctx, syncObj, 1);
266 @@ -371,11 +382,12 @@ _mesa_GetSynciv(GLsync sync, GLenum pname, GLsizei bufSize, GLsizei *length,
269 GET_CURRENT_CONTEXT(ctx);
270 - struct gl_sync_object *const syncObj = (struct gl_sync_object *) sync;
271 + struct gl_sync_object *syncObj;
275 - if (!_mesa_validate_sync(ctx, syncObj)) {
276 + syncObj = _mesa_get_sync(ctx, sync, true);
278 _mesa_error(ctx, GL_INVALID_VALUE, "glGetSynciv (not a valid sync object)");
281 @@ -409,6 +421,7 @@ _mesa_GetSynciv(GLsync sync, GLenum pname, GLsizei bufSize, GLsizei *length,
284 _mesa_error(ctx, GL_INVALID_ENUM, "glGetSynciv(pname=0x%x)\n", pname);
285 + _mesa_unref_sync_object(ctx, syncObj, 1);
289 @@ -421,4 +434,6 @@ _mesa_GetSynciv(GLsync sync, GLenum pname, GLsizei bufSize, GLsizei *length,
290 if (length != NULL) {
294 + _mesa_unref_sync_object(ctx, syncObj, 1);
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 *);
302 _mesa_free_sync_data(struct gl_context *);
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);
310 -_mesa_unref_sync_object(struct gl_context *ctx, struct gl_sync_object *syncObj);
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,
318 extern GLboolean GLAPIENTRY
319 _mesa_IsSync(GLsync sync);