Updated patch with some other places with a potential leak.
On Thu, Mar 24, 2016 at 3:52 AM Guilherme Melo <gqm...@gmail.com> wrote:
> 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 <gqm...@gmail.com> 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 <a...@nwnk.net>:
>>
>>> 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 3dc360e2998392d62af7c8272819e2c91350d183 Mon Sep 17 00:00:00 2001
From: Guilherme Quentel Melo <gqm...@gmail.com>
Date: Thu, 24 Mar 2016 03:13:30 -0300
Subject: [PATCH] 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 <gqm...@gmail.com>
---
glx/glxcmds.c | 20 ++++++++++++++++++--
glx/glxext.c | 18 +++++++++++++++---
2 files changed, 33 insertions(+), 5 deletions(-)
diff --git a/glx/glxcmds.c b/glx/glxcmds.c
index 561faeb..ffc6e9b 100644
--- a/glx/glxcmds.c
+++ b/glx/glxcmds.c
@@ -188,6 +188,11 @@ validGlxDrawable(ClientPtr client, XID id, int type, int access_mode,
void
__glXContextDestroy(__GLXcontext * context)
{
+ __GLXcontext *lastglxc;
+ if (lastGLContext != NULL && lastGLContext != context) {
+ lastglxc = (__GLXcontext*)lastGLContext;
+ (*lastglxc->loseCurrent) (lastglxc);
+ }
lastGLContext = NULL;
}
@@ -563,7 +568,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;
@@ -659,13 +664,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..b4fab20 100644
--- a/glx/glxext.c
+++ b/glx/glxext.c
@@ -117,7 +117,7 @@ static int glxBlockClients;
static Bool
DrawableGone(__GLXdrawable * glxPriv, XID xid)
{
- __GLXcontext *c, *next;
+ __GLXcontext *c, *next, *lastglxc;
if (glxPriv->type == GLX_DRAWABLE_WINDOW) {
/* If this was created by glXCreateWindow, free the matching resource */
@@ -139,6 +139,10 @@ DrawableGone(__GLXdrawable * glxPriv, XID xid)
c->hasUnflushedCommands = GL_FALSE;
/* just force a re-bind the next time through */
(*c->loseCurrent) (c);
+ if (lastGLContext != NULL && lastGLContext != c) {
+ lastglxc = (__GLXcontext*)lastGLContext;
+ (*lastglxc->loseCurrent) (lastglxc);
+ }
lastGLContext = NULL;
}
if (c->drawPriv == glxPriv)
@@ -280,7 +284,7 @@ glxClientCallback(CallbackListPtr *list, void *closure, void *data)
NewClientInfoRec *clientinfo = (NewClientInfoRec *) data;
ClientPtr pClient = clientinfo->client;
__GLXclientState *cl = glxGetClient(pClient);
- __GLXcontext *c, *next;
+ __GLXcontext *c, *next, *lastglxc;
switch (pClient->clientState) {
case ClientStateRunning:
@@ -293,6 +297,10 @@ glxClientCallback(CallbackListPtr *list, void *closure, void *data)
next = c->next;
if (c->currentClient == pClient) {
c->loseCurrent(c);
+ if (lastGLContext != NULL && lastGLContext != c) {
+ lastglxc = (__GLXcontext*)lastGLContext;
+ (*lastglxc->loseCurrent) (lastglxc);
+ }
lastGLContext = NULL;
c->currentClient = NULL;
FreeResourceByType(c->id, __glXContextRes, FALSE);
@@ -434,7 +442,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 +477,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.5.0
_______________________________________________
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
