Re: [libvirt] libvirt-0.9.5 availability of rc2

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.