[PATCH] Don't free font closures prematurely (#31501)
James Jones
jajones at nvidia.com
Fri Jul 29 12:31:12 PDT 2011
This patch isn't a correct fix. It just avoids the crash. I wish I could
remember why. There was some discussion about this on IRC between Adam
Jackson and I a long time ago that basically concluded the correct fix is
"really hard." If anyone keeps really old IRC logs around (around 8 or 9
months back), that would have made a good update in the bug report. Sorry
I didn't follow up.
Thanks,
-James
nvpublic
On 7/29/11 11:23 AM, "Jeremy Huddleston" <jeremyhu at freedesktop.org> wrote:
>This patch is attached to
>https://bugs.freedesktop.org/show_bug.cgi?id=31501 , but so far nobody
>has bothered to send it to xorg-devel ... I have not tested it and am
>just forwarding it along for comment since it reportedly addresses this
>crasher.
>
>
>From 59616dde4a04d407db76d55b06bcc82d646f42cd Mon Sep 17 00:00:00 2001
>From: James Jones <jajones at nvidia.com>
>Date: Thu, 9 Dec 2010 16:14:05 -0800
>Subject: [PATCH] Don't free font closures prematurely (#31501)
>X-NVConfidentiality: public
>
>When doing Xinerama, font ops are dispatched across
>backend screens. To avoid sleeping a client more than
>once in these cases, logic was added to avoid sleeping and
>destroy the closure structure when the client is already
>asleep. However, the operation isn't always completed
>when this cleanup is performed. To freeing prematurely,
>only free the closure data if the operation is finished,
>or if the operation never slept. The latter catches the
>xinerama case.
>
>Signed-off-by: James Jones <jajones at nvidia.com>
>---
> dix/dixfonts.c | 102
>+++++++++++++++++++++++++++++++++++----------------
> include/closestr.h | 5 +++
> 2 files changed, 75 insertions(+), 32 deletions(-)
>
>diff --git a/dix/dixfonts.c b/dix/dixfonts.c
>index ccb4627..c0ae28b 100644
>--- a/dix/dixfonts.c
>+++ b/dix/dixfonts.c
>@@ -239,6 +239,8 @@ doOpenFont(ClientPtr client, OFclosurePtr c)
> *newname;
> int newlen;
> int aliascount = 20;
>+ Bool fromDispatch = c->from_dispatch;
>+ Bool finished = FALSE;
> /*
> * Decide at runtime what FontFormat to use.
> */
>@@ -270,6 +272,8 @@ doOpenFont(ClientPtr client, OFclosurePtr c)
>
> BitmapFormatScanlineUnit8;
>
>+ c->from_dispatch = FALSE;
>+
> if (client->clientGone)
> {
> if (c->current_fpe < c->num_fpes)
>@@ -374,13 +378,16 @@ bail:
> c->fontid, FontToXError(err));
> }
> ClientWakeup(c->client);
>+ finished = TRUE;
> xinerama_sleep:
>- for (i = 0; i < c->num_fpes; i++) {
>- FreeFPE(c->fpe_list[i]);
>+ if (finished || fromDispatch) {
>+ for (i = 0; i < c->num_fpes; i++) {
>+ FreeFPE(c->fpe_list[i]);
>+ }
>+ free(c->fpe_list);
>+ free(c->fontname);
>+ free(c);
> }
>- free(c->fpe_list);
>- free(c->fontname);
>- free(c);
> return TRUE;
> }
>
>@@ -461,6 +468,7 @@ OpenFont(ClientPtr client, XID fid, Mask flags,
>unsigned lenfname, char *pfontna
> c->num_fpes = num_fpes;
> c->fnamelen = lenfname;
> c->flags = flags;
>+ c->from_dispatch = TRUE;
> c->non_cachable_font = cached;
>
> (void) doOpenFont(client, c);
>@@ -592,6 +600,10 @@ doListFontsAndAliases(ClientPtr client, LFclosurePtr
>c)
> char *bufptr;
> char *bufferStart;
> int aliascount = 0;
>+ Bool fromDispatch = c->from_dispatch;
>+ Bool finished = FALSE;
>+
>+ c->from_dispatch = FALSE;
>
> if (client->clientGone)
> {
>@@ -667,7 +679,7 @@ doListFontsAndAliases(ClientPtr client, LFclosurePtr
>c)
> ((pointer) c->client, fpe, &name, &namelen, &tmpname,
> &resolvedlen, c->current.private);
> if (err == Suspended) {
>- if (ClientIsAsleep(client))
>+ if (!ClientIsAsleep(client))
> ClientSleep(client,
> (ClientSleepProcPtr)doListFontsAndAliases,
> c);
>@@ -822,13 +834,16 @@ finish:
>
> bail:
> ClientWakeup(client);
>+ finished = TRUE;
> xinerama_sleep:
>- for (i = 0; i < c->num_fpes; i++)
>- FreeFPE(c->fpe_list[i]);
>- free(c->fpe_list);
>- free(c->savedName);
>- FreeFontNames(names);
>- free(c);
>+ if (finished || fromDispatch) {
>+ for (i = 0; i < c->num_fpes; i++)
>+ FreeFPE(c->fpe_list[i]);
>+ free(c->fpe_list);
>+ free(c->savedName);
>+ FreeFontNames(names);
>+ free(c);
>+ }
> free(resolved);
> return TRUE;
> }
>@@ -880,6 +895,7 @@ ListFonts(ClientPtr client, unsigned char *pattern,
>unsigned length,
> c->current.list_started = FALSE;
> c->current.private = 0;
> c->haveSaved = FALSE;
>+ c->from_dispatch = TRUE;
> c->savedName = 0;
> doListFontsAndAliases(client, c);
> return Success;
>@@ -891,6 +907,8 @@ doListFontsWithInfo(ClientPtr client, LFWIclosurePtr
>c)
> FontPathElementPtr fpe;
> int err = Successful;
> char *name;
>+ Bool fromDispatch = c->from_dispatch;
>+ Bool finished = FALSE;
> int namelen;
> int numFonts;
> FontInfoRec fontInfo,
>@@ -902,6 +920,8 @@ doListFontsWithInfo(ClientPtr client, LFWIclosurePtr
>c)
> int aliascount = 0;
> xListFontsWithInfoReply finalReply;
>
>+ c->from_dispatch = FALSE;
>+
> if (client->clientGone)
> {
> if (c->current.current_fpe < c->num_fpes)
>@@ -1095,13 +1115,16 @@ finish:
> WriteSwappedDataToClient(client, length, &finalReply);
> bail:
> ClientWakeup(client);
>+ finished = TRUE;
> xinerama_sleep:
>- for (i = 0; i < c->num_fpes; i++)
>- FreeFPE(c->fpe_list[i]);
>- free(c->reply);
>- free(c->fpe_list);
>- free(c->savedName);
>- free(c);
>+ if (finished || fromDispatch) {
>+ for (i = 0; i < c->num_fpes; i++)
>+ FreeFPE(c->fpe_list[i]);
>+ free(c->reply);
>+ free(c->fpe_list);
>+ free(c->savedName);
>+ free(c);
>+ }
> return TRUE;
> }
>
>@@ -1150,6 +1173,7 @@ StartListFontsWithInfo(ClientPtr client, int
>length, unsigned char *pattern,
> c->current.private = 0;
> c->savedNumFonts = 0;
> c->haveSaved = FALSE;
>+ c->from_dispatch = TRUE;
> c->savedName = 0;
> doListFontsWithInfo(client, c);
> return Success;
>@@ -1171,6 +1195,10 @@ doPolyText(ClientPtr client, PTclosurePtr c)
> FontPathElementPtr fpe;
> GC *origGC = NULL;
> int itemSize = c->reqType == X_PolyText8 ? 1 : 2;
>+ Bool fromDispatch = c->from_dispatch;
>+ Bool finished = FALSE;
>+
>+ c->from_dispatch = FALSE;
>
> if (client->clientGone)
> {
>@@ -1417,16 +1445,19 @@ bail:
> if (ClientIsAsleep(client))
> {
> ClientWakeup(c->client);
>+ finished = TRUE;
> xinerama_sleep:
>- ChangeGC(NullClient, c->pGC, clearGCmask, clearGC);
>+ if (finished || fromDispatch) {
>+ ChangeGC(NullClient, c->pGC, clearGCmask, clearGC);
>
>- /* Unreference the font from the scratch GC */
>- CloseFont(c->pGC->font, (Font)0);
>- c->pGC->font = NullFont;
>+ /* Unreference the font from the scratch GC */
>+ CloseFont(c->pGC->font, (Font)0);
>+ c->pGC->font = NullFont;
>
>- FreeScratchGC(c->pGC);
>- free(c->data);
>- free(c);
>+ FreeScratchGC(c->pGC);
>+ free(c->data);
>+ free(c);
>+ }
> }
> return TRUE;
> }
>@@ -1462,6 +1493,10 @@ doImageText(ClientPtr client, ITclosurePtr c)
> int err = Success, lgerr; /* err is in X error, not font error,
>space */
> FontPathElementPtr fpe;
> int itemSize = c->reqType == X_ImageText8 ? 1 : 2;
>+ Bool fromDispatch = c->from_dispatch;
>+ Bool finished = FALSE;
>+
>+ c->from_dispatch = FALSE;
>
> if (client->clientGone)
> {
>@@ -1571,16 +1606,19 @@ bail:
> if (ClientIsAsleep(client))
> {
> ClientWakeup(c->client);
>+ finished = TRUE;
> xinerama_sleep:
>- ChangeGC(NullClient, c->pGC, clearGCmask, clearGC);
>+ if (finished || fromDispatch) {
>+ ChangeGC(NullClient, c->pGC, clearGCmask, clearGC);
>
>- /* Unreference the font from the scratch GC */
>- CloseFont(c->pGC->font, (Font)0);
>- c->pGC->font = NullFont;
>+ /* Unreference the font from the scratch GC */
>+ CloseFont(c->pGC->font, (Font)0);
>+ c->pGC->font = NullFont;
>
>- FreeScratchGC(c->pGC);
>- free(c->data);
>- free(c);
>+ FreeScratchGC(c->pGC);
>+ free(c->data);
>+ free(c);
>+ }
> }
> return TRUE;
> }
>diff --git a/include/closestr.h b/include/closestr.h
>index ab18ef9..994263f 100644
>--- a/include/closestr.h
>+++ b/include/closestr.h
>@@ -53,6 +53,7 @@ typedef struct _OFclosure {
> XID fontid;
> char *fontname;
> int fnamelen;
>+ Bool from_dispatch;
> FontPtr non_cachable_font;
> } OFclosureRec;
>
>@@ -78,6 +79,7 @@ typedef struct _LFWIclosure {
> LFWIstateRec saved;
> int savedNumFonts;
> Bool haveSaved;
>+ Bool from_dispatch;
> char *savedName;
> } LFWIclosureRec;
>
>@@ -91,6 +93,7 @@ typedef struct _LFclosure {
> LFWIstateRec current;
> LFWIstateRec saved;
> Bool haveSaved;
>+ Bool from_dispatch;
> char *savedName;
> int savedNameLen;
> } LFclosureRec;
>@@ -109,6 +112,7 @@ typedef struct _PTclosure {
> CARD8 reqType;
> XID did;
> int err;
>+ Bool from_dispatch;
> } PTclosureRec;
>
> /* ImageText */
>@@ -123,5 +127,6 @@ typedef struct _ITclosure {
> int yorg;
> CARD8 reqType;
> XID did;
>+ Bool from_dispatch;
> } ITclosureRec;
> #endif /* CLOSESTR_H */
>--
>1.7.1
>
More information about the xorg-devel
mailing list