[Xcb] GetWMSizeHints

[Vincent Torri]

On Tue, 31 May 2005, Jamey Sharp wrote:

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

sorry for this, i mainly use the gnu coding style. It has become a habit
to use it. Just one thinkg: if the indentation is not very a big problem
for me (unless it's more than 5 characters), i think that the code is more
readable when there is a space between the name of the function and the
open parenthesis.

i'll try to think about that for my next commits.

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

 so, the 'value' is at the end of the reply. Ok.

>       * C has copy-by-value semantics: this code has no effect on the
>         SizeHints structure provided by the caller.

ok, the memcpy is needed, as i free the reply

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

ok.

> 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).

i'll look at it. it was the first thing i've wrote. I think it needs some
bug fixing :)

thanks

Vincent