[libvirt] libvirt-0.9.5 availability of rc2

[Daniel P. Berrange]

On Tue, Sep 20, 2011 at 12:03:00PM +0800, Daniel Veillard wrote:
> On Mon, Sep 19, 2011 at 08:15:18AM -0500, Adam Litke wrote:
> > On Mon, Sep 19, 2011 at 04:04:04PM +0800, Daniel Veillard wrote:
> > > On Sun, Sep 18, 2011 at 09:37:22AM -0500, Adam Litke wrote:
> > >   Hum, I wonder if remoteRelayDomainEventBlockJob shouldn't strdup the
> > > path string instead of using it directly in the
> > > remote_domain_event_block_job_msg block. As a result since we now
> > > free the datapointed by the xdr message within
> > > remoteDispatchDomainEventSend() , this errors wasn't shown before but
> > > leads to a double free now.
> > > 
> > > BTW it seems we don't check all allocations in the xdr code (on purpose
> > > ?) for example make_nonnull_domain() doesn't check a strdup.
> > > 
> > > Could you check the following patch ?
> > 
> > Yep, this seems to fix the problem (and an extra check with valgrind shows no
> > memory leaks.  Although I haven't verified it, the functions:
> > 
> >    remoteRelayDomainEventIOError
> >    remoteRelayDomainEventIOErrorReason
> >    remoteRelayDomainEventGraphics
> > 
> > appear to have the same problem as well.
> 
>   Right though they do far more allocations. I ended up pushing the
> following patch which tries to be a bit better on handling memory
> allocation error, but it's not fully complete yet:
> 
> 
> Fix crash on events due to allocation errors
> 
> remoteRelayDomainEventBlockJob, remoteRelayDomainEventIOError,
> remoteRelayDomainEventIOErrorReason and remoteRelayDomainEventGraphics
> were using const string directly in rpc structure, before calling
> remoteDispatchDomainEventSend(). But that routine now frees up all
> the pointed allocated memory from the rpc structure and we end up
> with a double free.
> This now strdup() all the strings passed and provide mem_error goto
> labels to be used when an allocation error occurs.
> Note that the cleanup isn't completely finished because all relaying
> function also call make_nonnull_domain() which also allocate a string
> and never handle the error case. This patches doesn't try to address
> this as this is only error correctness a priori and touches far more
> functions in this module:
> 
> * daemon/remote.c: fix string allocations and memory error handling
>   for remoteRelayDomainEventBlockJob, remoteRelayDomainEventIOError,
>   remoteRelayDomainEventIOErrorReason and remoteRelayDomainEventGraphics
> 
> diff --git a/daemon/remote.c b/daemon/remote.c
> index 45244f8..74e759a 100644
> --- a/daemon/remote.c
> +++ b/daemon/remote.c
> @@ -234,9 +234,13 @@ static int remoteRelayDomainEventIOError(virConnectPtr conn ATTRIBUTE_UNUSED,
>  
>      /* build return data */
>      memset(&data, 0, sizeof data);
> +    data.srcPath = strdup(srcPath);
> +    if (data.srcPath == NULL)
> +        goto mem_error;
> +    data.devAlias = strdup(devAlias);
> +    if (data.devAlias == NULL)
> +        goto mem_error;
>      make_nonnull_domain(&data.dom, dom);
> -    data.srcPath = (char*)srcPath;
> -    data.devAlias = (char*)devAlias;
>      data.action = action;
>  
>      remoteDispatchDomainEventSend(client, remoteProgram,
> @@ -244,6 +248,11 @@ static int remoteRelayDomainEventIOError(virConnectPtr conn ATTRIBUTE_UNUSED,
>                                    (xdrproc_t)xdr_remote_domain_event_io_error_msg, &data);
>  
>      return 0;
> +mem_error:
> +    virReportOOMError();
> +    virFree(data.srcPath);
> +    virFree(data.devAlias);
> +    return -1;

s/virFree/VIR_FREE/ throughout all this code.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|