Re: [PATCH] accel/tcg: assert insn_idx will always be valid before plugin_inject_cb

["Alex Bennée" <alex.bennee@linaro.org>]

Richard Henderson <richard.henderson@linaro.org> writes:

> On 9/3/21 7:59 AM, Alex Bennée wrote:
>> Coverity doesn't know enough about how we have arranged our plugin TCG
>> ops to know we will always have incremented insn_idx before injecting
>> the callback. Let us assert it for the benefit of Coverity and protect
>> ourselves from accidentally breaking the assumption and triggering
>> harder to grok errors deeper in the code if we attempt a negative
>> indexed array lookup.
>> Fixes: Coverity 1459509
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>   accel/tcg/plugin-gen.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>> diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
>> index 88e25c6df9..b38aa1bb36 100644
>> --- a/accel/tcg/plugin-gen.c
>> +++ b/accel/tcg/plugin-gen.c
>> @@ -820,10 +820,9 @@ static void pr_ops(void)
>>   static void plugin_gen_inject(const struct qemu_plugin_tb *plugin_tb)
>>   {
>>       TCGOp *op;
>> -    int insn_idx;
>> +    int insn_idx = -1;
>>         pr_ops();
>> -    insn_idx = -1;
>>       QSIMPLEQ_FOREACH(op, &tcg_ctx->plugin_ops, plugin_link) {
>>           enum plugin_gen_from from = op->args[0];
>>           enum plugin_gen_cb type = op->args[1];
>> @@ -834,6 +833,7 @@ static void plugin_gen_inject(const struct qemu_plugin_tb *plugin_tb)
>>               type == PLUGIN_GEN_ENABLE_MEM_HELPER) {
>>               insn_idx++;
>>           }
>> +        g_assert(from == PLUGIN_GEN_FROM_TB || insn_idx >= 0);
>>           plugin_inject_cb(plugin_tb, op, insn_idx);
>
> Hmm.  This is the single caller of plugin_inject_cb.
>
> I think we could simplify all of this by inlining it, so that we can
> put these blocks into their proper place within the switch.

I guess. This was the simplest form for calming coverity but I can
experiment with more refactoring.

> Also, existing strageness in insn_idx being incremented for non-insns?

It shouldn't be - it's just using the presence of the memory
instrumentation as a proxy for the start of a instruction and dealing
with the slightly different start of block boundary. 

> Should it be named something else?  I haven't looked at how it's
> really used in the end.

We need the insn idx to find the registered callbacks for a given
instruction later. We could maybe embed a metadata TCGOp that could
track this for us but that might make TCG a bit more confusing as it
doesn't really need that information?

>
>
> r~


-- 
Alex Bennée