[xserver] dix: don't free() stack memory

Submitted by Eric Engestrom on March 13, 2018, 10:56 a.m.

Details

Message ID 20180313105652.3992-1-eric.engestrom@imgtec.com
State New
Series "dix: don't free() stack memory"
Headers show

Commit Message

Eric Engestrom March 13, 2018, 10:56 a.m.
In function ‘doImageText’,
    inlined from ‘ImageText’ at dix/dixfonts.c:1513:5:
dix/dixfonts.c:1492:9: warning: attempt to free a non-heap object ‘local_closure’ [-Wfree-nonheap-object]
         free(c);
         ^

Signed-off-by: Eric Engestrom <eric.engestrom@imgtec.com>
---
 dix/dixfonts.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

Patch hide | download patch | download mbox

diff --git a/dix/dixfonts.c b/dix/dixfonts.c
index cca92ed2791ccf262017..c48034dd41426b47915d 100644
--- a/dix/dixfonts.c
+++ b/dix/dixfonts.c
@@ -1498,19 +1498,20 @@  int
 ImageText(ClientPtr client, DrawablePtr pDraw, GC * pGC, int nChars,
           unsigned char *data, int xorg, int yorg, int reqType, XID did)
 {
-    ITclosureRec local_closure;
+    ITclosureRec *local_closure = malloc(sizeof(*local_closure));
 
-    local_closure.client = client;
-    local_closure.pDraw = pDraw;
-    local_closure.pGC = pGC;
-    local_closure.nChars = nChars;
-    local_closure.data = data;
-    local_closure.xorg = xorg;
-    local_closure.yorg = yorg;
-    local_closure.reqType = reqType;
-    local_closure.did = did;
+    local_closure->client = client;
+    local_closure->pDraw = pDraw;
+    local_closure->pGC = pGC;
+    local_closure->nChars = nChars;
+    local_closure->data = data;
+    local_closure->xorg = xorg;
+    local_closure->yorg = yorg;
+    local_closure->reqType = reqType;
+    local_closure->did = did;
 
-    (void) doImageText(client, &local_closure);
+    (void) doImageText(client, local_closure);
+    free(local_closure);
     return Success;
 }
 

Comments

Michel Dänzer March 13, 2018, 11:09 a.m.
On 2018-03-13 11:56 AM, Eric Engestrom wrote:
> In function ‘doImageText’,
>     inlined from ‘ImageText’ at dix/dixfonts.c:1513:5:
> dix/dixfonts.c:1492:9: warning: attempt to free a non-heap object ‘local_closure’ [-Wfree-nonheap-object]
>          free(c);
>          ^
> 
> Signed-off-by: Eric Engestrom <eric.engestrom@imgtec.com>
> ---
>  dix/dixfonts.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/dix/dixfonts.c b/dix/dixfonts.c
> index cca92ed2791ccf262017..c48034dd41426b47915d 100644
> --- a/dix/dixfonts.c
> +++ b/dix/dixfonts.c
> @@ -1498,19 +1498,20 @@ int
>  ImageText(ClientPtr client, DrawablePtr pDraw, GC * pGC, int nChars,
>            unsigned char *data, int xorg, int yorg, int reqType, XID did)
>  {
> -    ITclosureRec local_closure;
> +    ITclosureRec *local_closure = malloc(sizeof(*local_closure));
>  
> -    local_closure.client = client;
> -    local_closure.pDraw = pDraw;
> -    local_closure.pGC = pGC;
> -    local_closure.nChars = nChars;
> -    local_closure.data = data;
> -    local_closure.xorg = xorg;
> -    local_closure.yorg = yorg;
> -    local_closure.reqType = reqType;
> -    local_closure.did = did;
> +    local_closure->client = client;
> +    local_closure->pDraw = pDraw;
> +    local_closure->pGC = pGC;
> +    local_closure->nChars = nChars;
> +    local_closure->data = data;
> +    local_closure->xorg = xorg;
> +    local_closure->yorg = yorg;
> +    local_closure->reqType = reqType;
> +    local_closure->did = did;
>  
> -    (void) doImageText(client, &local_closure);
> +    (void) doImageText(client, local_closure);
> +    free(local_closure);

If the free(c) in the compiler warning above is hit, this is a
double-free, isn't it?
Eric Engestrom March 13, 2018, 11:58 a.m.
On Tuesday, 2018-03-13 12:09:40 +0100, Michel Dänzer wrote:
> On 2018-03-13 11:56 AM, Eric Engestrom wrote:
> > In function ‘doImageText’,
> >     inlined from ‘ImageText’ at dix/dixfonts.c:1513:5:
> > dix/dixfonts.c:1492:9: warning: attempt to free a non-heap object ‘local_closure’ [-Wfree-nonheap-object]
> >          free(c);
> >          ^
> > 
> > Signed-off-by: Eric Engestrom <eric.engestrom@imgtec.com>
> > ---
> >  dix/dixfonts.c | 23 ++++++++++++-----------
> >  1 file changed, 12 insertions(+), 11 deletions(-)
> > 
> > diff --git a/dix/dixfonts.c b/dix/dixfonts.c
> > index cca92ed2791ccf262017..c48034dd41426b47915d 100644
> > --- a/dix/dixfonts.c
> > +++ b/dix/dixfonts.c
> > @@ -1498,19 +1498,20 @@ int
> >  ImageText(ClientPtr client, DrawablePtr pDraw, GC * pGC, int nChars,
> >            unsigned char *data, int xorg, int yorg, int reqType, XID did)
> >  {
> > -    ITclosureRec local_closure;
> > +    ITclosureRec *local_closure = malloc(sizeof(*local_closure));
> >  
> > -    local_closure.client = client;
> > -    local_closure.pDraw = pDraw;
> > -    local_closure.pGC = pGC;
> > -    local_closure.nChars = nChars;
> > -    local_closure.data = data;
> > -    local_closure.xorg = xorg;
> > -    local_closure.yorg = yorg;
> > -    local_closure.reqType = reqType;
> > -    local_closure.did = did;
> > +    local_closure->client = client;
> > +    local_closure->pDraw = pDraw;
> > +    local_closure->pGC = pGC;
> > +    local_closure->nChars = nChars;
> > +    local_closure->data = data;
> > +    local_closure->xorg = xorg;
> > +    local_closure->yorg = yorg;
> > +    local_closure->reqType = reqType;
> > +    local_closure->did = did;
> >  
> > -    (void) doImageText(client, &local_closure);
> > +    (void) doImageText(client, local_closure);
> > +    free(local_closure);
> 
> If the free(c) in the compiler warning above is hit, this is a
> double-free, isn't it?

Yes, yes it is...  :facepalm:

I'll look at the code more closely to figure out when the free is
needed, but I just saw this warning and had a look, this isn't code I'm
familiar with *at all*, so I might just end up giving up if I can't
figure it out easily enough :/

> 
> 
> -- 
> Earthling Michel Dänzer               |               http://www.amd.com
> Libre software enthusiast             |             Mesa and X developer
Keith Packard March 13, 2018, 4:22 p.m.
Eric Engestrom <eric.engestrom@imgtec.com> writes:

> I'll look at the code more closely to figure out when the free is
> needed, but I just saw this warning and had a look, this isn't code I'm
> familiar with *at all*, so I might just end up giving up if I can't
> figure it out easily enough :/

There's no free of stack memory in this path, but the compiler can't
figure it out. The free path is only hit if the client had been put to
sleep to wait for the font, in which case the original stack structure
would have been copied to allocated memory.

Hrm. It might be easy to fix this by simply creating a new function to
pass to ClientSleep which does the free and leave doImageText out of
that part.

Alternatively, leave the code alone and stick some comments or
instructions for the compiler to not complain.