Re: [Evolution-hackers] Is this a locking bug?

[Not Zed <notzed ximian com>]

This is close but not quite right.

The lock is only required around the e_memchunk_free(), as e_mempools and e_memchunks aren't locked (maybe that was a bad decision but it's the api).

I've committed this slightly altered fix below.  Thanks for chasing this down.

 Michael

2004-01-05  Not Zed  <NotZed Ximian com>

	* e-memory.c (e_mempool_destroy): Fix from Zan Lynx
	<zlynx acm org> to lock the memchunk before freeing the pool
	header.

void e_mempool_destroy(MemPool *pool)
{
	if (pool) {
		e_mempool_flush(pool, 1);
#ifdef G_THREADS_ENABLED
		g_static_mutex_lock(&mempool_mutex);
#endif
		e_memchunk_free(mempool_memchunk, pool);
#ifdef G_THREADS_ENABLED
		g_static_mutex_unlock(&mempool_mutex);
#endif
	}
}

On Mon, 2003-12-29 at 00:18 -0700, Zan Lynx wrote:
> I think I found a locking bug in e-util/e-memory.c.  This bug, if it is
> this bug, has been crashing Evolution on me several times a day when
> running on a SMP system.  I patched it, and Evolution hasn't crashed
> yet, so I'm posting it.
>
> I'm not sure if the lock needs to include the e_mempool_flush or not.  I
> just tried to follow the locking in e_mempool_new.
>
> This is Evolution 1.4.5, compiled on Gentoo.  
>
> Here is the patch:
> --- /tmp/e-memory.c	2003-12-29 00:07:18.691690575 -0700
> +++ e-util/e-memory.c	2003-12-28 22:05:51.000000000 -0700
> @@ -534,10 +534,16 @@
>   **/
>  void e_mempool_destroy(MemPool *pool)
>  {
> +#ifdef G_THREADS_ENABLED
> +	g_static_mutex_lock(&mempool_mutex);
> +#endif
>  	if (pool) {
>  		e_mempool_flush(pool, 1);
>  		e_memchunk_free(mempool_memchunk, pool);
>  	}
> +#ifdef G_THREADS_ENABLED
> +	g_static_mutex_unlock(&mempool_mutex);
> +#endif
>  }
>
> -- 
> Zan Lynx <zlynx acm org>