[Xcb] GetWMSizeHints

[Jamey Sharp]

On Sat, 2005-05-14 at 14:23 +0200, Vincent Torri wrote:
> On Fri, 13 May 2005, Jamey Sharp wrote:
> > On Fri, 2005-05-13 at 08:50 +0200, Vincent Torri wrote:
> > > I have written the code for GetWMSizeHints. I have a question about it:
> > > in Xlib, they take into account some pre-ICCCM structures for the
> > > SizeHints structure (that counts 15 members, instead of 18 for ICCCM v.
> > > 1).
> > > Should I take that into account too ? Or else the value returned by
> > > XCBGetPropertyValue is always for ICCCM v. 1 ?
> >
> > If it's easy to be backwards-compatible, just do it. It's always better
> > to support more existing configurations if the costs of doing so are
> > low.
> 
> Ok, it's done, I've added them in cvs. Please, have a look at it when you
> get some time.

OK, I've just looked over your ICCCM changes. The ideas are fine; thanks
for the improvements! I also checked a recent small patch you committed
to XCBImage.

Minor things: I'm a little annoyed that you changed the whitespace
around all of the function prototypes, particularly as part of a commit
with real functionality changes, but that's alright. I'm also a little
annoyed that the code you added didn't use the same indentation style as
the rest of the source file, so I've committed a fix for that. These are
no big deal.

The issue I really want to point out here, because it seems to have
confused most of the people who have tried to use XCB, is that functions
like XCBGetPropertyValue return pointers into the reply structure that
you hand them. The idiom I've used in the past, as you apparently
noticed while reading XCL source, is to memmove the variable-length part
of a reply up to the beginning of the reply, after I'm done with all the
fixed-length data. This saves a malloc and a free and avoids accessing
free'd memory, leaking things, and trying to free inside the middle of a
malloc'd block.

Let's look at some code, and I'll try to make that clearer.

int
GetWMSizeHints (XCBConnection *c,
                XCBWINDOW      window,
                XCBATOM        property,
                SizeHints     *hints,
                long          *supplied)
{
        /* ... */
        if (/* property is OK */)
        {
                unsigned char *prop;
                long length = XCBGetPropertyValueLength (rep);
                /* FIXME: in GetProp.c of xcl, one move the memory.
                 * Should we do that too ? */
                prop = (unsigned char *) XCBGetPropertyValue (rep);
                prop[length] = '\0';
                hints = (SizeHints *)strdup (prop);
                /* ... */
                free (rep);
                
                return 1;
        }
        
        hints = NULL;
        free (rep);
        
        return 0;
}

To begin with, there are a couple of bugs here.
      * prop[length] is past the end of the allocated memory, so the
        above code will overrun its buffer every time.
      * C has copy-by-value semantics: this code has no effect on the
        SizeHints structure provided by the caller.

If this is really the right API for this function, these bugs can be
fixed pretty simply. Quit trying to assign pointers to hints:

        int ret = 0;
        /* ... */
        {
                memcpy(hints, XCBGetPropertyValue(rep),
                        XCBGetPropertyValueLength(rep));
                /* ... */
                ret = 1;
        }
        free(rep);
        return ret;

However, I'd probably be inclined to return a pointer to a
newly-allocated SizeHints structure instead. The necessary changes for
that are pretty straightforward.

In the case of XCBImageGet, which has similar problems, I'd use the
memmove approach, which unfortunately I don't have time to write about
right now. I just want to point out for the moment that you can't free
the return value of XCBGetImageData, as that doesn't point at the
beginning of a malloc'd block, and you don't want to free the reply
here, as the newly-allocated XCBImage structure has a pointer to the
insides of the reply (which, when destroyed, will result in attempting
to free the middle of a malloc'd block).

--Jamey