| Message ID | 20210903145938.1321571-1-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | accel/tcg: assert insn_idx will always be valid before plugin_inject_cb | expand |
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~
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
On 9/13/21 3:06 AM, Alex Bennée wrote: >> 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? We have a metadata op for marking instruction boundaries already: INDEX_op_insn_start. r~
Richard Henderson <richard.henderson@linaro.org> writes: > On 9/13/21 3:06 AM, Alex Bennée wrote: >>> 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? > > We have a metadata op for marking instruction boundaries already: > INDEX_op_insn_start. Hmm so we have a separate list for speedy access: /* list to quickly access the injected ops */ QSIMPLEQ_HEAD(, TCGOp) plugin_ops; I wonder if we should drop that and just scan QTAILQ_HEAD(, TCGOp) ops so we can be properly aligned with the current instruction. Alternatively we could just emit INDEX_op_insn_start to the plugin list as well? > > > r~ -- Alex Bennée
On 9/13/21 7:06 AM, Alex Bennée wrote: > Hmm so we have a separate list for speedy access: > > /* list to quickly access the injected ops */ > QSIMPLEQ_HEAD(, TCGOp) plugin_ops; > > I wonder if we should drop that and just scan QTAILQ_HEAD(, TCGOp) ops > so we can be properly aligned with the current instruction. > Alternatively we could just emit INDEX_op_insn_start to the plugin list > as well? I suspect that just scanning the whole list would be easiest. Then you don't need to maintain your own separate list. r~
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); } pr_ops();
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(-) -- 2.30.2