[PATCH v3 0/4] xprop: Free various allocated memory

[Eirik Byrkjeflot Anonsen]

Eirik Byrkjeflot Anonsen <eirik at eirikba.org> writes:

> Adam Jackson <ajax at nwnk.net> writes:
>
>> On Thu, 2015-07-23 at 11:17 +0200, Eirik Byrkjeflot Anonsen wrote:
>>
>>> I'm not particularly surprised, but given that the patch is made and is
>>> largely trivial it would be nice to get it into the official code. Does
>>> anyone have any idea how to make that happen?
>>
>> Apologies for the delay here, I've been going through the patchwork
>> backlog and saw this series.  1/4 and 3/4 look fine, I've got ahead and
>> merged them:
>>
>> remote: I: patch #50243 updated using rev 4f748e3d2b1368ec0590a413ba5f7addc5e3344f.
>> remote: I: patch #49787 updated using rev dee1d0c1316b1c62c6c62d6f0f4b13685e8e6630.
>> remote: I: 2 patch(es) updated to state Accepted.
>> To ssh://git.freedesktop.org/git/xorg/app/xprop
>>    b0ae4b9..dee1d0c  master -> master
>>
>> The other two are a little wonky at first glance.  Is there a reason to
>> use that static trick instead of just freeing at the end of the
>> function?  From the very briefest of glances it doesn't look like the
>> pointers leak outside their caller, but maybe I missed it.
>>
>> - ajax
>
> Patch 2 is the "prop" variable in Get_Window_Property_Data_And_Type, I
> think. Note that the function ends with:
>
>     return (const char*)prop;
>
> So I rather think it would be a bad idea to free it before the end of
> the function :)
>
> Patch 4 is the "result" variable in Format_Icons(). That function also
> ends with:
>
>    return result;
>
> So yes, I feel pretty confident in saying that these should be kept
> alive beyond the end of their respective functions.
>
> If I remember correctly, these patches frees up memory that ends up
> being returned by Get_Property_Data_And_Type(). So the nice solution
> would be to free them in Show_Prop() once that function is done with it.
> However, the various data that may be returned from
> Get_Property_Data_And_Type() is not all allocated in the same way. If I
> remember correctly there are at least some values that are malloced,
> some that should be XFreed and some that are statically allocated.
>
> An alternative solution would be to change all functions that return
> data through Get_Property_Data_And_Type() to return malloced data. Then
> we could add the free() in Show_Prop(). But that is rather a bigger
> change.
>
> eirik

Were my arguments unconvincing, or did you just get busy with other
things again :)

(Or maybe I should add some comments to the code pointing out those
issues I mentioned above? Maybe something like "This variabe is returned
back out through Get_Property_Data_And_type() to Show_Prop() where it is
used and then never touched again. Unfortunately, at that point, we
don't know whether to free() it, XFree() it or not free it at all. So to
avoid building up leaks over time, free it the next time we get here.")

eirik