On Wed, 29 Jan 2014 21:10:52 -0500 Michael Blumenkrantz <michael.blumenkra...@gmail.com> said:
> On Thu, 30 Jan 2014 11:03:48 +0900 > Carsten Haitzler (The Rasterman) <ras...@rasterman.com> wrote: > > > On Wed, 29 Jan 2014 20:39:41 -0500 Michael Blumenkrantz > > <michael.blumenkra...@gmail.com> said: > > > > > On Thu, 30 Jan 2014 10:32:26 +0900 > > > Carsten Haitzler (The Rasterman) <ras...@rasterman.com> wrote: > > > > > > > On Wed, 29 Jan 2014 20:23:46 -0500 Michael Blumenkrantz > > > > <michael.blumenkra...@gmail.com> said: > > > > > > > > > On Wed, 29 Jan 2014 16:01:10 -0800 > > > > > Carsten Haitzler <ras...@rasterman.com> wrote: > > > > > > > > > > > raster pushed a commit to branch master. > > > > > > > > > > > > http://git.enlightenment.org/core/enlightenment.git/commit/?id=83397e1bde51830016e9a0f8e6482fc91bb4c50c > > > > > > > > > > > > commit 83397e1bde51830016e9a0f8e6482fc91bb4c50c > > > > > > Author: Carsten Haitzler (Rasterman) <ras...@rasterman.com> > > > > > > Date: Thu Jan 30 08:55:28 2014 +0900 > > > > > > > > > > > > fix segv where comp_data is null but still accessed > > > > > > > > > > > > it seems i have an override-redirect window just off the > > > > > > bottom-right of my screen - i think its the scim input panel status. > > > > > > what happens is it is "managed" by comp but then deleted > > > > > > (_e_comp_x_hook_client_del called), BUT _e_comp_x_object_add is > > > > > > called with a deferred event for that client to add it again > > > > > > (likely this is a race) which finds he client in a state of not > > > > > > having comp_data as the E_FREE in _e_comp_x_hook_client_del() frees > > > > > > it and sets it to NULL. move the comp_data free to the actual > > > > > > client free (which is the last time a client is valid at all) > > > > > > solves this. > > > > > > --- > > > > > > src/bin/e_client.c | 1 + > > > > > > src/bin/e_comp_x.c | 1 - > > > > > > 2 files changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > > > > > > > I guess I'll have to take a look at this. Freeing comp_data in > > > > > _e_client_free () is wrong in all cases since it's compositor-specific > > > > > and guarantees that data will leak. > > > > > > > > eh? how does it guarantee it will leak? a client free frees the > > > > comp_data - you'd otherwise have to never free a client to never free > > > > the comp data set ON the client itself. so yes - it's a leak if clients > > > > are never freed... whic is a leak in and of itself. > > > > > > > > simply - it's not a leak now. sure - it's not freed int he same domain > > > > where it's allocated - that is true, but the problem is that if that is > > > > left there it is ACCESSED in the same domain after being freed - the > > > > client is still attached to an object, but the comp data has been freed > > > > for the client. so the logical options are to enusre the objects are > > > > deleted at the point the comp data is freed OR defer the comp data free > > > > until the client is actually freed/cleaned up. i chose option 2 since > > > > it had no leaks and was the least amount of changes. > > > > > > > > > > I think you are misunderstanding what I'm saying. The comp_data struct > > > itself won't leak, but everything in it will leak since this is opaque. > > > Freeing it there is a poor workaround to fixing the underlying bug. > > > > look at the patch... everything inside of it is free/deleted in the same > > spot. just the struct ITSELF it deferred to be freed later. so we keep an > > empty "comp_data" struct. yeah - i should have nul/0'd out > > comp_data->damage as that is freed but still left as a valid resource ref. > > all of the rest of the deletion still happens in _e_comp_x_hook_client_del > > () EXCEPT the free of the comp_data struct itself. THAT is the only thing > > not freed there - and JUST that is deferred until client free so the > > comp_data ptr stays valid for as long as it is assumed to exist/be valid (i > > read all the code accessing it - all the code ASSUMES that if you have a > > client struct, comp_data is non-null and valid at all times). > > Right, I understand what you're saying and what your patch does. It functions > as expected now, in X, and I'm not disputing that. > > Given that comp_data is opaque, however, it's possible (and I have > considered) using other types of objects for this. If you were to call free() > on something like an Eina_Hash, you would leak data. oh sure - if it were something other than a malloced struct - yes. it'd have problems. as i see it there now... that is what it is and thats the only type/kind of data there. yes - could add a free func that is attached when comp_data is and thus he who allocs/attaches comp_data provides the cleanup mechanism, or fundamentally re-jig how all of this is glued together/done. fo now at least the simplest solution was to just move the free to the last possible point of use of comp_data. :) -- ------------- Codito, ergo sum - "I code, therefore I am" -------------- The Rasterman (Carsten Haitzler) ras...@rasterman.com ------------------------------------------------------------------------------ WatchGuard Dimension instantly turns raw network data into actionable security intelligence. It gives you real-time visual feedback on key security issues and trends. Skip the complicated setup - simply import a virtual appliance and go from zero to informed in seconds. http://pubads.g.doubleclick.net/gampad/clk?id=123612991&iu=/4140/ostg.clktrk _______________________________________________ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
