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

[Keith Packard]

Eric Engestrom  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.

-- 
-keith


signature.asc
Description: PGP signature
___
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 xserver] dix: don't free() stack memory

[Eric Engestrom]

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 
> > ---
> >  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, _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
___
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 xserver] dix: don't free() stack memory

[Michel Dänzer]

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 
> ---
>  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, _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?


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
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

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

[Eric Engestrom]

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 
---
 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, _closure);
+(void) doImageText(client, local_closure);
+free(local_closure);
 return Success;
 }
 
-- 
Cheers,
  Eric

___
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