Whoops. Patch updated on a different thread. -John Sheu
On Sun, Apr 3, 2016 at 11:33 PM, Alejandro Piñeiro <[email protected]> wrote: > On 04/04/16 08:11, Alejandro Piñeiro wrote: >> On 02/04/16 01:52, John Sheu wrote: >>> XMesaVisual.vishandle is being unconditionally overwritten every time >>> glXChooseVisual/glXGetVisualFromFBConfig returns a new XVisualInfo >>> instance, causing a memory leak. >>> --- >>> src/mesa/drivers/x11/fakeglx.c | 26 ++++++++------------------ >>> 1 file changed, 8 insertions(+), 18 deletions(-) >>> >>> diff --git a/src/mesa/drivers/x11/fakeglx.c b/src/mesa/drivers/x11/fakeglx.c >>> index 2f4d9669..d62d5abd 100644 >>> --- a/src/mesa/drivers/x11/fakeglx.c >>> +++ b/src/mesa/drivers/x11/fakeglx.c >>> @@ -1241,16 +1241,11 @@ Fake_glXChooseVisual( Display *dpy, int screen, int >>> *list ) >>> >>> xmvis = choose_visual(dpy, screen, list, GL_FALSE); >>> if (xmvis) { >>> -#if 0 >>> - return xmvis->vishandle; >>> -#else >>> - /* create a new vishandle - the cached one may be stale */ >> This comments points that xmvis->vishandle is a cached value that needs >> to be updated. And taking a quick look on the code, is used in other parts. >> >>> - xmvis->vishandle = malloc(sizeof(XVisualInfo)); >>> - if (xmvis->vishandle) { >>> - memcpy(xmvis->vishandle, xmvis->visinfo, sizeof(XVisualInfo)); >> The old code created space for vishandle and assigned the value of >> xmvis->visinfo. >> >>> + XVisualInfo* visinfo = malloc(sizeof(XVisualInfo)); >>> + if (visinfo) { >>> + memcpy(visinfo, xmvis->visinfo, sizeof(XVisualInfo)); >> But the new code created a new XVisualInfo, and returned it, without >> updating xmis>vishandle, so the new code seems to do something >> different. Additionally, I don't see how this would prevent the memory >> leak, as a new XVisualInfo is created each time it is called, without >> being assigned to the structure. >> >> If you want to prevent the leak, I think that it would be better to >> maintain the same code, but freeing the previous xmvis->vishandle if it >> is different to NULL. > > Ok. Nevermind, I see that you remove vishandle on a following patch, and > frees the newly created visinfo. But that means that the leak is has the > old code is not fixed with this patch, but with the following one. > Probably it would be better to update the commit summary and description. > >> >>> } >>> - return xmvis->vishandle; >>> -#endif >>> + return visinfo; >>> } >>> else >>> return NULL; >>> @@ -1974,16 +1969,11 @@ Fake_glXGetVisualFromFBConfig( Display *dpy, >>> GLXFBConfig config ) >>> { >>> if (dpy && config) { >>> XMesaVisual xmvis = (XMesaVisual) config; >>> -#if 0 >>> - return xmvis->vishandle; >>> -#else >>> - /* create a new vishandle - the cached one may be stale */ >>> - xmvis->vishandle = malloc(sizeof(XVisualInfo)); >>> - if (xmvis->vishandle) { >>> - memcpy(xmvis->vishandle, xmvis->visinfo, sizeof(XVisualInfo)); >>> + XVisualInfo* visinfo = malloc(sizeof(XVisualInfo)); >>> + if (visinfo) { >>> + memcpy(visinfo, xmvis->visinfo, sizeof(XVisualInfo)); >>> } >>> - return xmvis->vishandle; >>> -#endif >>> + return visinfo; >>> } >> Ditto. >> >> And as I mentioned on my review to your "xlib: fix memory leak on >> Display close" patch, this code is C&P in other places of the code. >> >> BRb >>> else { >>> return NULL; >> _______________________________________________ >> mesa-dev mailing list >> [email protected] >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev > _______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
