Module: Mesa Branch: master Commit: feb53912f8d8c29594a9fdff914d78bb36d6d56b URL: http://cgit.freedesktop.org/mesa/mesa/commit/?id=feb53912f8d8c29594a9fdff914d78bb36d6d56b
Author: Steinar H. Gunderson <[email protected]> Date: Tue Feb 2 01:16:51 2016 +0100 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 <[email protected]> Cc: "11.0 11.1" <[email protected]> Signed-off-by: Marek Olšák <[email protected]> --- src/mesa/main/objectlabel.c | 13 +++++-- src/mesa/main/shared.c | 2 +- src/mesa/main/syncobj.c | 89 ++++++++++++++++++++++++++------------------- src/mesa/main/syncobj.h | 11 ++---- 4 files changed, 66 insertions(+), 49 deletions(-) diff --git a/src/mesa/main/objectlabel.c b/src/mesa/main/objectlabel.c index 41f370c..b622d6a 100644 --- 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 c37b31d..b9f7bb6 100644 --- 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 c1b2d3b..be758dd 100644 --- 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 5d510e8..ea4a712 100644 --- 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); _______________________________________________ mesa-commit mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/mesa-commit
