Richard Henderson <richard.hender...@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.ben...@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
