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

[Richard Henderson]

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.

Also, existing strageness in insn_idx being incremented for non-insns?  Should it be named 
something else?  I haven't looked at how it's really used in the end.


r~