Re: [PATCH glx] glx: avoid memory leak when using indirect rendering
On Mon, May 9, 2016 at 4:41 PM Adam Jackson wrote: > On Mon, 2016-04-04 at 19:57 +, Guilherme Melo wrote: > > Updated patch with some other places with a potential leak. > > Apologies for being so slow to look at this. > > Unfortunately I think it's incorrect. lastGLContext is not necessarily > of __GLXcontext type - glamor sets it a glamor_ctx *, for example - so > passing it to ->loseCurrent isn't always going to work. > > I think your first patch that only did the loseCurrent from > __glXForceCurrent was closer to correct. > Thanks for the feedback. I did not know that lastGLContext was shared with glamor. So it really seems the first patch is more appropriate. I just thought it was confusing because it actually needs to lose the same context that it is currently making current. I'll resend the patch soon. Guilherme ___ 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
Re: [PATCH glx] glx: avoid memory leak when using indirect rendering
On Mon, 2016-04-04 at 19:57 +, Guilherme Melo wrote: > Updated patch with some other places with a potential leak. Apologies for being so slow to look at this. Unfortunately I think it's incorrect. lastGLContext is not necessarily of __GLXcontext type - glamor sets it a glamor_ctx *, for example - so passing it to ->loseCurrent isn't always going to work. I think your first patch that only did the loseCurrent from __glXForceCurrent was closer to correct. - ajax ___ 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
Re: [PATCH glx] glx: avoid memory leak when using indirect rendering
Updated patch with some other places with a potential leak.
On Thu, Mar 24, 2016 at 3:52 AM Guilherme Melo 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 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 :
>>
>>> 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
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
---
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 != prev
Re: [PATCH glx] glx: avoid memory leak when using indirect rendering
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 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 :
>
>> 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
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
---
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
Re: [PATCH glx] glx: avoid memory leak when using indirect rendering
> 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 :
> 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
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH glx] glx: avoid memory leak when using indirect rendering
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
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
