On Wed, Sep 21, 2011 at 10:55:37AM +0100, Daniel P. Berrange wrote: > 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. Yeah Eric pointed that out, I already commited his patch and the associated make syntax-check test :-) Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list