I've finally got some time to rewrite this patch and now the solution makes more sense. I'm sending as an attachment. I also have some tests on a github repository to check this bug. I don't know if it is ok to post the link here though.
Guilherme On Tue, Dec 8, 2015 at 4:48 PM Guilherme Melo <[email protected]> wrote: > > This should be lastGLContext->loseCurrent(lastGLContext) I think. cx > > > Yes, you are right. It seems the right thing to do, but actually this > should be done every place where lastGLContext is set, right? > It seems to me that every place where lastGLContext is set is a potential > leak. > > While you're at it, it'd be nice to move that big block comment into the >> commit message > > > I'll do that, thanks. > > > Guilherme > > > > 2015-12-08 14:44 GMT-02:00 Adam Jackson <[email protected]>: > >> On Fri, 2015-12-04 at 14:16 -0200, Guilherme Quentel Melo wrote: >> >> > diff --git a/glx/glxext.c b/glx/glxext.c >> > index e41b881..16b8039 100644 >> > --- a/glx/glxext.c >> > +++ b/glx/glxext.c >> > @@ -469,6 +469,24 @@ __glXForceCurrent(__GLXclientState * cl, >> GLXContextTag tag, int *error) >> > >> > /* Make this context the current one for the GL. */ >> > if (!cx->isDirect) { >> > + /* >> > + ** If we are forcing the context it means someone already >> called makeCurrent before. If >> > + ** we just call makeCurrent again, the drawable of this >> context will be left with one >> > + ** refcount more forever and will never be freed. >> > + ** >> > + ** This situation happens when there are many X clients using >> GL: >> > + ** >> > + ** 1 - client1: calls glXMakeCurrent >> > + ** >> > + ** 2 - client2: calls glXMakeCurrent >> > + ** This is the first context switch for this client. So >> old_context_tag=0 >> > + ** >> > + ** 3 - client1: calls glXRender >> > + ** For the client, its context is already current. >> > + ** For the server side lastGLContext points to client2's >> context. So the execution path >> > + ** will get here. >> > + */ >> > + (*cx->loseCurrent) (cx); >> >> This should be lastGLContext->loseCurrent(lastGLContext) I think. cx >> here is the current context from client2's perspective, you want to >> release client1's context. >> >> While you're at it, it'd be nice to move that big block comment into >> the commit message and just note /* drop previous client's context */ >> or similar in the code. >> >> - ajax > > >
From b921bbbdb9d3d79d1ff487b4f3270fb675ba69d2 Mon Sep 17 00:00:00 2001 From: Guilherme Quentel Melo <[email protected]> Date: Thu, 24 Mar 2016 03:13:30 -0300 Subject: [PATCH glx] glx: avoid memory leak when using indirect rendering When multiple processes are using GL with indirect rendering a race condition can make drawables refcount never drop to zero. This situation could happen when there are many X clients using indirect GLX: 1 - client1: calls glXMakeCurrent 2 - client2: calls glXMakeCurrent This is the first context switch for this client. So old_context_tag=0 3 - client1: calls glXRender For the client, its context is already current. For the server side lastGLContext points to client2's context. Signed-off-by: Guilherme Quentel Melo <[email protected]> --- glx/glxcmds.c | 15 +++++++++++++-- glx/glxext.c | 6 +++++- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/glx/glxcmds.c b/glx/glxcmds.c index f5f2bab..94b31b8 100644 --- a/glx/glxcmds.c +++ b/glx/glxcmds.c @@ -551,7 +551,7 @@ DoMakeCurrent(__GLXclientState * cl, { ClientPtr client = cl->client; xGLXMakeCurrentReply reply; - __GLXcontext *glxc, *prevglxc; + __GLXcontext *glxc, *prevglxc, *lastglxc = NULL; __GLXdrawable *drawPriv = NULL; __GLXdrawable *readPriv = NULL; int error; @@ -642,13 +642,24 @@ DoMakeCurrent(__GLXclientState * cl, if (!(*prevglxc->loseCurrent) (prevglxc)) { return __glXError(GLXBadContext); } - lastGLContext = NULL; if (!prevglxc->isDirect) { prevglxc->drawPriv = NULL; prevglxc->readPriv = NULL; } } + /* + ** lastGLContext may be different than prevglxc, so we need lose it to + ** avoid a memory leak + */ + if (lastGLContext != NULL) { + lastglxc = (__GLXcontext*)lastGLContext; + if (!lastglxc->isDirect && lastglxc != prevglxc) { + (*lastglxc->loseCurrent) (lastglxc); + } + lastGLContext = NULL; + } + if ((glxc != 0) && !glxc->isDirect) { glxc->drawPriv = drawPriv; diff --git a/glx/glxext.c b/glx/glxext.c index e41b881..c556489 100644 --- a/glx/glxext.c +++ b/glx/glxext.c @@ -434,7 +434,7 @@ GlxExtensionInit(void) __GLXcontext * __glXForceCurrent(__GLXclientState * cl, GLXContextTag tag, int *error) { - __GLXcontext *cx; + __GLXcontext *cx, *lastglxc = NULL; /* ** See if the context tag is legal; it is managed by the extension, @@ -469,6 +469,10 @@ __glXForceCurrent(__GLXclientState * cl, GLXContextTag tag, int *error) /* Make this context the current one for the GL. */ if (!cx->isDirect) { + if (lastGLContext != NULL) { + lastglxc = (__GLXcontext*)lastGLContext; + (*lastglxc->loseCurrent) (lastglxc); + } lastGLContext = cx; if (!(*cx->makeCurrent) (cx)) { /* Bind failed, and set the error code. Bummer */ -- 2.1.4
_______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
