GNOME Bugzilla – Bug 724852
Memory leaks in seamless clone
Last modified: 2014-03-20 09:31:43 UTC
Created attachment 269872 [details] patch to fix memory leaks in seamless clone The leaks detected and fixed herein were discovered using the "Leaks" template in Apple's "Instruments" tool. At least according to Instruments, there are completely no leaks anymore after applying the fixes in this commit.
(In reply to comment #0) > Created an attachment (id=269872) [details] > patch to fix memory leaks in seamless clone > > The leaks detected and fixed herein were discovered using the "Leaks" template > in Apple's "Instruments" tool. At least according to Instruments, there are > completely no leaks anymore after applying the fixes in this commit. I'd say that at least the two arrays allocated at: https://git.gnome.org/browse/gegl/tree/seamless-clone/sc-sample.c#n90 are two other memory leaks. Plus I found also that I had to call g_list_free (parts) here: https://git.gnome.org/browse/gegl/tree/libs/poly2tri-c/poly2tri-c/refine/cdt.c#n322
Doesn't this create a use-after-free? current = next; next = p2tr_point_edge_cw (P, current); + p2tr_edge_unref(next); Without looking deeper than the diff it seems like you should be unref'ing the old current not next.
Hi Massimo That's true, thanks for pointing these out. I wonder what is the correct practice to include these in the patch, should I edit and re-upload the original patch in some way, or should I create a new patch and upload it? Sorry for the stupid sounding question, but I'm a bit new to world class open source development. Hi Sabo The reason for unref-ing next is because p2tr_point_edge_cw acts like other p2tr_get_* functions which refs the return value before returning. The assumption of p2tr_get_* like functions is that it's the caller's responsibility to unref it when it no longer needs it. If we unref only current, the last next that does not satisfy the condition in the while loop while (next != g_queue_peek_head (&cluster->edges) would not be unrefed resulting in memory leaks. (In reply to comment #1) > (In reply to comment #0) > > Created an attachment (id=269872) [details] [details] > > patch to fix memory leaks in seamless clone > > > > The leaks detected and fixed herein were discovered using the "Leaks" template > > in Apple's "Instruments" tool. At least according to Instruments, there are > > completely no leaks anymore after applying the fixes in this commit. > > > I'd say that at least the two arrays allocated at: > > https://git.gnome.org/browse/gegl/tree/seamless-clone/sc-sample.c#n90 > > are two other memory leaks. > > Plus I found also that I had to call g_list_free (parts) here: > > https://git.gnome.org/browse/gegl/tree/libs/poly2tri-c/poly2tri-c/refine/cdt.c#n322
Created attachment 269975 [details] [review] additional fixes to two more memory leaks pointed out Massimo This patch fixes two additional memory leaks pointed out by Massimo. It is intended to be used along with the first patch.
(In reply to comment #3) > Hi Massimo > > That's true, thanks for pointing these out. I wonder what is the correct > practice to include these in the patch, should I edit and re-upload the > original patch in some way, or should I create a new patch and upload it? Sorry > for the stupid sounding question, but I'm a bit new to world class open source > development. Hi Awaw, both ways are fine: when you attach a new patch to a bug report, you have a check-box to mark previous patches as obsoletes so when you chose to replace a previous patch, you have to check it.
> The assumption of p2tr_get_* like functions is that it's the caller's > responsibility to unref it when it no longer needs it. But you're unrefing it while it's still needed. It isn't going to crash here because ‘next’ is still part of the larger structure, but it's still wrong. > If we unref only current, the last next that does not satisfy the condition in the while loop > > while (next != g_queue_peek_head (&cluster->edges) > > would not be unrefed resulting in memory leaks. You should unref current just before you replace it's value and also after you exit the loop.
Hi Massimo Got it, I've created a new patch which is intended to used along with first patch, thanks for your explanation. Hi Sabo Right, I finally understand your point, unref-ing current is conceptually more correct. I have refactored that function a bit to unref the current and next pointers this way.
Created attachment 270032 [details] [review] refactor to unref current which is conceptually more correct
Looks OK to me, Massimo?
Created attachment 270373 [details] [review] quick hack rendering the following composition (which used to render fine): <gegl> <gegl:crop width='512' height='512' /> <svg:dst-over> <gegl:clone ref='input'/> </svg:dst-over> <gegl:seamless-clone xoff='32' yoff='32'> <gegl:crop width='448' height='448' /> <gegl:checkerboard color1='#48d' color2='#f82' /> </gegl:seamless-clone> <gegl:crop id='input' width='512' height='512' /> <gegl:checkerboard color1='#22d' color2='#dd2' /> </gegl> now crashes bin/.libs/gegl, so I'm not able to verify the patches attached above. in the quick hack I attached, I'm not sure of the sign of offset_x and _y, but anyway something like that is necessary.
Sorry, I am building the library on Xcode, and am not fully used to testing with the gegl xml files. I tried to build and run a test based on this composition, following the instructions on http://www.gegl.org/development.html , but failed with the error configure: error: cannot find install-sh, install.sh, or shtool in "." "./.." "./../.." I guess I'm lacking some GNU libraries on my Macbook. Do you have any advice on running this composition on Mac OSX directly? Aside from that, I'm a bit perplexed by this crash, since the changes here are not concerned with gegl buffers. I wonder does the same composition render well without all the changes here?
(In reply to comment #11) > > Aside from that, I'm a bit perplexed by this crash, since the changes here are > not concerned with gegl buffers. I wonder does the same composition render well > without all the changes here? No, sorry, the crash is happening also without your changes. I don't know what's necessary to build the library with Xcode
Even without the patches attached to this bug report, using the seamless clone tool crashes GIMP as well:
+ Trace 233242
Thanks for the trace, from the logic inside P2TR_MESH_RENDER_FROM_CACHE, it seems that it might be an array out of bounds exception, assuming there's a mechanism to do this at runtime. Let me see if I can borrow a linux box from the College of Engineering IT center.
I opened another bug report (725604) for the crash, with a simpler test case, because it is not related to these memory leaks or fixes. https://bugzilla.gnome.org/show_bug.cgi?id=725604
commit dd3660fd44185464765a9af174a0302a25dc5372 Author: awaw fumin <awawfumin@gmail.com> Date: Fri Feb 21 11:45:24 2014 +0800 operations: Fix memory leaks in seamless clone The leaks detected and fixed herein were discovered using the "Leaks" template in Apple's "Instruments" tool. At least according to Instruments, there are completely no leaks anymore after applying the fixes in this commit. libs/poly2tri-c/poly2tri-c/refine/cdt.c | 2 ++ libs/poly2tri-c/poly2tri-c/refine/cluster.c | 11 +++++++++-- libs/poly2tri-c/poly2tri-c/refine/delaunay-terminator.c | 5 +++++ libs/poly2tri-c/poly2tri-c/refine/mesh.c | 10 ++++++---- seamless-clone/sc-context.c | 8 ++++++++ seamless-clone/sc-sample.c | 3 +++ 6 files changed, 33 insertions(+), 6 deletions(-)
Running the composition attached to comment 10 with git master now bin/gegl fails printing: GEGL-point.c:ERROR:point.c:215:p2tr_point_unref: assertion failed: (self->refcount > 0) comparing current master with a patch I had in a branch here, it seems that I had to delete this line: https://git.gnome.org/browse/gegl/tree/seamless-clone/sc-sample.c#n249 (and the comment preceeding it) to avoid the abort following the assertion failure. And I also had to call: p2tr_mesh_clear (self->mesh); also here: https://git.gnome.org/browse/gegl/tree/seamless-clone/sc-context.c#n190 as it is now done here: https://git.gnome.org/browse/gegl/tree/seamless-clone/sc-context.c#n776
Right, the `points` attribute of GeglScMeshSampling is simply inserted using `g_ptr_array_add` without `p2tr_point_ref`, therefore a `p2tr_point_unref` shouldn't be needed when freeing. It's true we need to also call `p2tr_mesh_clear` in sc-context.c#n190. Thanks for these comments, are they included somewhere or should I make a patch for them?
yes please, it is very kind of you if you make the patch
Created attachment 271505 [details] [review] fix a wrong unreferencing and memory leak on the mesh No problem, attached is a patch with the two discussed changes.
commit 3e38920b41a0d1f163a843303301c976391f0df4 Author: awaw fumin <awawfumin@gmail.com> Date: Tue Mar 11 14:22:28 2014 +0800 Fix a bad unref and a memory leak seamless-clone/sc-context.c | 3 ++- seamless-clone/sc-sample.c | 2 -- 2 files changed, 2 insertions(+), 3 deletions(-)
The patch attached here: https://bugzilla.gnome.org/attachment.cgi?id=270032 that was later committed, did not remove a call to p2tr_edge_unref before the second while loop: - current = E; + current = p2tr_edge_ref (E); next = p2tr_point_edge_cw (P, current); - p2tr_edge_unref(next); while (next != g_queue_peek_head (&cluster->edges) ... - current = E; + current = p2tr_edge_ref (E); next = p2tr_point_edge_ccw(P, current); p2tr_edge_unref(next); while (next != g_queue_peek_tail (&cluster->edges) and now the seamless-clone tool crashes GIMP with: ** GEGL-edge.c:ERROR:edge.c:88:p2tr_edge_unref: assertion failed: (self->refcount > 0) when copy/pasting an elliptic selection. Unfortunately my previous test did not exercise this code path.
Created attachment 272315 [details] [review] remove wrong unreferencing Sorry, my bad, I shouldn't have made this mistake. Attached is a patch that fixes this bug.
commit 0c92e87f97bee0551950b06ab9da8009f345e46c Author: awaw fumin <awawfumin@gmail.com> Date: Wed Mar 19 01:13:47 2014 +0800 seamless-close: Fix extra unref