Re: Possible bug in tools/libxl/libxl.c -- Variable passed by reference not set in one possible case

[Ian Campbell <Ian.Campbell@citrix.com>]

On Fri, 2014-10-10 at 11:54 +0100, Wei Liu wrote:
> On Fri, Oct 10, 2014 at 10:30:06AM +0100, Ian Campbell wrote:
> [...]
> > > 
> > > The way the callers use it prevents the issue you described from
> > > happening -- they only call this function when they can't read those
> > > values from xenstore -- if those values are already in xenstore this
> > > function won't get called.
> > 
> > At least the callers in libxl__get_free_memory_slack and
> > libxl__get_memory_target look to be racy with someone else writing these
> > nodes to me.
> > 
> 
>  libxl__fill_dom0_memory_info uses transaction already, that should
>  avoid race?

I don't think so.

The caller is:
	read target (perhaps in a transaction)
	if (!target)	
		***
		if ( call fill_dom0_memory(&target) < 0 ) /* uses a transaction */
			return
		use target
	}

If another process dives in at *** and writes target then
fill_dom0_memory will return 0 but not initialise target, but the caller
will still (potentially) use it.

Even if it somehow transpires that through some careful coding in the
caller the use target never actually happens because of some other
reason it way to opaque as it is currently written.

I think the initialisation of current_target_memkb to zero in 
libxl_set_memory_target might be masking the issue here, preventing the
caller from (legitimately) complaining about a variable which is used
without initialisation

> > libxl_set_memory_target is a bit unclear but the fact that it ends the
> > transaction right before it calls libxl__fill_dom0_memory_info seems
> > suspicious.
> > 
> 
> To avoid having a transaction inside another transaction?

I suppose that might have been what the author was thinking, bit it
means that help if you are relying on that transaction to provide
atomicity you lose.

Ian