Re: Accessing previously freed data.

[Dan Williams <dcbw redhat com>]

On Thu, 2009-03-05 at 15:15 -0700, Drew Moseley wrote:
> Dan Williams wrote:
> > I looked over the code there again, and it's correct AFAICT.
> > g_slist_delete_link() in the sync_devices() function should be doing
> > what its intended to do; it removes one link from the linked list
> > priv->devices and frees the link.  It doesn't free the data of course,
> > but that should get correctly unref-ed right below.  And after that, the
> > device should no longer be in priv->devices at all.  That doesn't mean
> > there isn't a refcounting bug, but I'm still not quite sure what's
> > causing the invalid access later on...
> 
> Hi Dan,
> 
> Looks like the issue is that the delete_link call is incorrect since the node being passed in is from the copy of the list.  The patch below addresses this by building the new list by adding the items that exist and then deleting the old list.
> 
> Thoughts?

Excellent catch.  Thanks!  Will commit.

Dan

> Drew
> 
> 
> diff --git a/src/nm-manager.c b/src/nm-manager.c
> index 30660f1..9a8c1e8 100644
> --- a/src/nm-manager.c
> +++ b/src/nm-manager.c
> @@ -1479,12 +1479,11 @@ static void
>  sync_devices (NMManager *self)
>  {
>  	NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE (self);
> -	GSList *devices;
> +	GSList *devices = NULL;
>  	GSList *iter;
>  
>  	/* Remove devices which are no longer known to HAL */
> -	devices = g_slist_copy (priv->devices);
> -	for (iter = devices; iter; iter = iter->next) {
> +	for (iter = priv->devices; iter; iter = iter->next) {
>  		NMDevice *device = NM_DEVICE (iter->data);
>  		const char *udi = nm_device_get_udi (device);
>  
> @@ -1493,13 +1492,14 @@ sync_devices (NMManager *self)
>  				nm_device_set_managed (device, TRUE, NM_DEVICE_STATE_REASON_NOW_MANAGED);
>  			else
>  				nm_device_set_managed (device, FALSE, NM_DEVICE_STATE_REASON_NOW_UNMANAGED);
> +			devices = g_slist_prepend(devices, device);
>  		} else {
> -			priv->devices = g_slist_delete_link (priv->devices, iter);
>  			remove_one_device (self, device);
>  		}
>  	}
>  
> -	g_slist_free (devices);
> +	g_slist_free (priv->devices);
> +	priv->devices = devices;
>  
>  	/* Get any new ones */
>  	nm_hal_manager_query_devices (priv->hal_mgr);
>