Re: [Qemu-devel] Tracking unfreed tcg temps

[Aurelien Jarno]

On Tue, Jan 11, 2011 at 11:55:33PM +0100, Aurelien Jarno wrote:
> On Tue, Jan 11, 2011 at 06:09:06AM -0600, Peter Maydell wrote:
> > The ARM target-arm/translate.c file has some code in it which tries to
> > track the number of TCG temporaries allocated during translation of an
> > ARM instruction and complain if they are not freed by the end of that
> > instruction. So new_tmp() allocates a temp with tcg_temp_new_i32() and
> > increments the count; dead_tmp() calls tcg_temp_free() and decrements
> > the count. If at the end of translating an instruction the count isn't
> > zero we print a warning:
> > 
> >   fprintf(stderr, "Internal resource leak before %08x\n", dc->pc);
> > 
> > However there are a lot of code paths which will trigger this warning;
> > generally these are for invalid encodings where we only notice that
> > the opcode is invalid after having loaded the input operands, so we've
> > allocated a temp but the generate-UNDEF-exception exit path doesn't
> > free it.
> > 
> > tcg/README says that failure to free a temporary is not very harmful,
> > because it just means more memory usage and slower translation. (On
> > the other hand there seems to be a hardcoded TCG_MAX_TEMPS limit on
> > the number of temporaries, which suggests that freeing temporaries is
> > important and the README is misleading?)
> 
> This temporary is only valid for a given TB, so the leak is not going to
> take more and more memory. On the other hand, if it is easy to trigger
> such leaks with non-priviledged instructions and reach TCG_MAX_TEMPS,
> this means that a simple user on a virtual machine can crash it. No risk
> of security issue, but at least a DoS.
> 
> Note also that our register spill strategy is not really optimized, so
> the generated code might be less optimized in such cases.
> 
> > So what's the best thing to do with this temporary-tracking code?
> > 
> > (1) just get rid of it as it is misguided
> 
> I think it is something important to make sure temp are correctly freed.
> OTOH, it's maybe not a good idea to expose such message to users.
> 
> > (2) tweak it so that we don't complain about non-freed temps if this
> > is the end of the TB anyway [since the invalid-encoding case will
> > always result in ending the TB]
> 
> That might be a temporary solution.
> 
> > (3) rework all the code which catches invalid encodings so that we can
> > identify undefined instructions before we have done any of the
> > preparatory loading of operands that is causing the warning to trigger
> > 
> > [If it is useful to track not-freed-temps like this shouldn't the
> > code be in tcg and not ad-hoc in target-arm anyway?]
> 
> I guess this is currently only done in target-arm, as it is loading
> registers a bit differently than other targets. Other targets, or at
> least some of them, tends to have a short par of code between
> tcg_temp_new() and tcg_temp_free(), so it's easier to verify manually.
> This is also probably related to the way the instruction space is split,
> and for sure thumb doesn't help here.

Looking at https://bugs.launchpad.net/qemu/+bug/702885 , it actually
seems that resource leak can be the reverse: tcg temps that are freed
twice. It can be something dangerous, at it can free the wrong temp.

Looking at the code, one of the problem of the ARM target is that some
functions free temps passed in arguments, some other don't. I am not
sure if it is best to let the caller or the callee free temp arguments,
but for sure it's best to have it consistent all along translate.c to 
avoid coding errors.

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
address@hidden                 http://www.aurel32.net