Race condition in sp_gradient_invalidate_vector()?

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

Race condition in sp_gradient_invalidate_vector()?

Ralf Stephan
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&group_id=93438
https://sourceforge.net/tracker/?func=detail&atid=604306&aid=1196143&group_id=93438


Regards,
ralf



-------------------------------------------------------
SF.Net email is sponsored by: Discover Easy Linux Migration Strategies
from IBM. Find simple to follow Roadmaps, straightforward articles,
informative Webcasts and more! Get everything you need to get up to
speed, fast. http://ads.osdn.com/?ad_id=7477&alloc_id=16492&op=click
_______________________________________________
Inkscape-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/inkscape-devel
Reply | Threaded
Open this post in threaded view
|

Re: Race condition in sp_gradient_invalidate_vector()?

MenTaLguY
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


-------------------------------------------------------
SF.Net email is sponsored by: Discover Easy Linux Migration Strategies
from IBM. Find simple to follow Roadmaps, straightforward articles,
informative Webcasts and more! Get everything you need to get up to
speed, fast. <a href="http://ads.osdn.com/?ad_idt77&alloc_id492&op=click">http://ads.osdn.com/?ad_idt77&alloc_id492&op=click
_______________________________________________
Inkscape-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/inkscape-devel
Reply | Threaded
Open this post in threaded view
|

Re: Race condition in sp_gradient_invalidate_vector()?

Ralf Stephan
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);
 }
 



-------------------------------------------------------
SF.Net email is sponsored by: Discover Easy Linux Migration Strategies
from IBM. Find simple to follow Roadmaps, straightforward articles,
informative Webcasts and more! Get everything you need to get up to
speed, fast. http://ads.osdn.com/?ad_id=7477&alloc_id=16492&op=click
_______________________________________________
Inkscape-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/inkscape-devel
Reply | Threaded
Open this post in threaded view
|

Re: Race condition in sp_gradient_invalidate_vector()?

Ralf Stephan
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);
 }
 


-------------------------------------------------------
SF.Net email is sponsored by: Discover Easy Linux Migration Strategies
from IBM. Find simple to follow Roadmaps, straightforward articles,
informative Webcasts and more! Get everything you need to get up to
speed, fast. http://ads.osdn.com/?ad_id=7477&alloc_id=16492&op=click
_______________________________________________
Inkscape-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/inkscape-devel
Reply | Threaded
Open this post in threaded view
|

Re: Race condition in sp_gradient_invalidate_vector()?

bulia byak
Thanks Ralf, I committed both patches and closed both bugs. Great job!

--
bulia byak
Inkscape. Draw Freely.
http://www.inkscape.org


-------------------------------------------------------
SF.Net email is sponsored by: Discover Easy Linux Migration Strategies
from IBM. Find simple to follow Roadmaps, straightforward articles,
informative Webcasts and more! Get everything you need to get up to
speed, fast. <a href="http://ads.osdn.com/?ad_idt77&alloc_id492&op=click">http://ads.osdn.com/?ad_idt77&alloc_id492&op=click
_______________________________________________
Inkscape-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/inkscape-devel
Reply | Threaded
Open this post in threaded view
|

Re: Race condition in sp_gradient_invalidate_vector()?

Peter Moulder
In reply to this post by Ralf Stephan
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;
>      }
>  


-------------------------------------------------------
SF.Net email is sponsored by: Discover Easy Linux Migration Strategies
from IBM. Find simple to follow Roadmaps, straightforward articles,
informative Webcasts and more! Get everything you need to get up to
speed, fast. http://ads.osdn.com/?ad_id=7477&alloc_id=16492&op=click
_______________________________________________
Inkscape-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/inkscape-devel
Reply | Threaded
Open this post in threaded view
|

Re: Race condition in sp_gradient_invalidate_vector()?

MenTaLguY
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

signature.asc (196 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Race condition in sp_gradient_invalidate_vector()?

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


-------------------------------------------------------
SF.Net email is sponsored by: Discover Easy Linux Migration Strategies
from IBM. Find simple to follow Roadmaps, straightforward articles,
informative Webcasts and more! Get everything you need to get up to
speed, fast. http://ads.osdn.com/?ad_id=7477&alloc_id=16492&op=click
_______________________________________________
Inkscape-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/inkscape-devel