Race condition in sp_gradient_invalidate_vector()?
Hello,
both bugs 1038594 and 1196143 appear to be 'fixed' by not g_free'ing gr->color. This is a text gradient buffer mangled in both bugs (in 1196143 you can see some destruction of the picture when repainting after dialog release). In 1038594, valgrind tells me about attempted reads from the same address which was freed before, and gtk likely reuses the memory, and this fast.
This would mean that there is a copy of the color buf somewhere that is not updated but I still am not well into the code.
I would propose memory handling of this buffer to be moved into GC, before the release, or using malloc/free if that works.
https://sourceforge.net/tracker/?func=detail&atid=604306&aid=1038594... https://sourceforge.net/tracker/?func=detail&atid=604306&aid=1196143...
Regards, ralf
Thank you for your hard work in tracking this one down!
I'll have a look and see what would be involved in using GC for the buffer.
In principle, the buffer referenced via gr->color could simply be allocated via
gr->color = new (GC::ATOMIC) char[whatever...];
But the garbage collector doesn't know anything about SPGradients (because SPObjects aren't managed by it), so the reference wouldn't get tracked.
I suspect we shall have to make SPGradient::color a refcounted object rather than a raw buffer.
That isn't necessarily exclusive with making it managed by the collector; we can use GC::Anchored. That way, references from GC-managed objects can be handled automatically, and references from "outside" can be reflected via refcounts.
As far as tracking down the other lingering reference to the color buffer, I'd definitely suggest looking at the NRArenaItem hierarchy if you haven't already.
-mental
OK this does it. Can someone please close both bugs 1038594 and 1196143 with this patch? I'd do it myself if you gave me permission.
And please also close bug 1226096. Thanks!
ralf
Explanation: the LGradient renderer, on its setup, gets a copy of gr->color but that isn't updated when gr->color is freed on redraw. We now check for that before rendering. Also the order of nulling+freeing is reversed to make sure there is no race condition (not sure if necessary but hey).
--- inkscape-20050701-2000/src/sp-gradient.cpp 2005-07-02 05:00:25.000000000 +0200 +++ inkscape-patched/src/sp-gradient.cpp 2005-07-02 12:00:23.999286048 +0200 @@ -774,9 +774,10 @@ { bool ret = false;
- if (gr->color) { - g_free(gr->color); + if (gr->color != NULL) { + void* tmp = gr->color; gr->color = NULL; + g_free(tmp); ret = true; }
@@ -1354,6 +1360,12 @@ { SPLGPainter *lgp = (SPLGPainter *) painter;
+ if (lgp->lg->color == NULL) + { + sp_gradient_ensure_colors (lgp->lg); + lgp->lgr.vector = lgp->lg->color; + } + nr_render((NRRenderer *) &lgp->lgr, pb, NULL); }
oops do not forget the radial gradients...
--- inkscape-20050701-2000/src/sp-gradient.cpp 2005-07-02 05:00:25.000000000 +0200 +++ inkscape-patched/src/sp-gradient.cpp 2005-07-02 12:46:11.000000000 +0200 @@ -1595,6 +1607,12 @@ { SPRGPainter *rgp = (SPRGPainter *) painter;
+ if (rgp->rg->color == NULL) + { + sp_gradient_ensure_colors (rgp->rg); + rgp->rgr.vector = rgp->rg->color; + } + nr_render((NRRenderer *) &rgp->rgr, pb, NULL); }
Thanks Ralf, I committed both patches and closed both bugs. Great job!
On Sat, Jul 02, 2005 at 12:18:52PM +0200, Ralf Stephan wrote:
Also the order of nulling+freeing is reversed to make sure there is no race condition (not sure if necessary but hey).
Surely this is unnecessary unless we are multi-threaded, or some other asynchronous event (signal handler?) can access gr->color here.
If either of those "unless" conditions is true, then I'd really like to be aware of it!
Otherwise, I'd like to reverse the below hunk, for readability.
pjrm.
--- inkscape-20050701-2000/src/sp-gradient.cpp 2005-07-02 05:00:25.000000000 +0200 +++ inkscape-patched/src/sp-gradient.cpp 2005-07-02 12:00:23.999286048 +0200 @@ -774,9 +774,10 @@ { bool ret = false;
- if (gr->color) {
g_free(gr->color);
- if (gr->color != NULL) {
void* tmp = gr->color; gr->color = NULL;g_free(tmp); ret = true;
On Mon, 2005-07-04 at 15:19 +1000, Peter Moulder wrote:
On Sat, Jul 02, 2005 at 12:18:52PM +0200, Ralf Stephan wrote:
Also the order of nulling+freeing is reversed to make sure there is no race condition (not sure if necessary but hey).
Surely this is unnecessary unless we are multi-threaded, or some other asynchronous event (signal handler?) can access gr->color here.
There aren't. Even if there were, reordering the operations wouldn't help. It'd be necessary to establish a memory barrier (e.g. using one of the standard locking primitives).
With no memory barrier established, compiler and CPU would be free to arbitrarily reorder the operations, and even without that, threads on other CPUs may see the operations totally differently because of cacheing.
Really it'd be necessary to establish a critical section around the whole bit, too, in the likely case that the code were racing with anything beyond another thread also trying to free the buffer.
-mental
On Mon, Jul 04, 2005 at 03:09:10AM -0400, MenTaLguY wrote:
On Mon, 2005-07-04 at 15:19 +1000, Peter Moulder wrote:
On Sat, Jul 02, 2005 at 12:18:52PM +0200, Ralf Stephan wrote:
Also the order of nulling+freeing is reversed to make sure there is no race condition (not sure if necessary but hey).
Surely this is unnecessary unless we are multi-threaded, or some other asynchronous event (signal handler?) can access gr->color here.
There aren't. Even if there were, reordering the operations wouldn't help.
OK, I've now reverted that part.
pjrm.
participants (5)
-
unknown＠example.com -
bulia byak -
MenTaLguY -
Peter Moulder -
Ralf Stephan