[glx] glx: avoid memory leak when using indirect rendering

Submitted by Guilherme Quentel Melo on March 24, 2016, 6:52 a.m.

Details

Message ID CACF+4B_tWue9ra0VEJrMi3oueQXT9yNhEj1Y5ab+MxE6ZyFiFA@mail.gmail.com
State Superseded
Headers show
Series "glx: avoid memory leak when using indirect rendering" ( rev: 2 ) in X.org (DEPRECATED - USE GITLAB)

Not browsing as part of any series.

Commit Message

Guilherme Quentel Melo March 24, 2016, 6:52 a.m. 
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 <gqmelo@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 <ajax@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
>
>
>

Patch hide | download patch | download mbox 

From b921bbbdb9d3d79d1ff487b4f3270fb675ba69d2 Mon Sep 17 00:00:00 2001
From: Guilherme Quentel Melo <gqmelo@gmail.com>
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 <gqmelo@gmail.com>
---
 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