On 6/29/25 1:50 PM, Alex Bennée wrote:
Yodel Eldar <yodel.el...@gmail.com> writes:Currently, execlog searches for a space separator between the instruction mnemonic and operands, but some disassemblers, e.g. Alpha's, use a tab separator instead; this results in a null pointer being passed as the haystack in g_strstr during a subsequent register search, i.e. undefined behavior, because of a missing null check. This patch adds tab to the separator search and a null check on the result. Also, existing, affected pointers are changed to const. Lastly, a break statement was added to immediately terminate the register search when a user-requested register is found in the current instruction as a trivial optimization, because searching for the remaining requested registers is unnecessary once one is found. Signed-off-by: Yodel Eldar <yodel.el...@gmail.com> --- contrib/plugins/execlog.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/contrib/plugins/execlog.c b/contrib/plugins/execlog.c index d67d010761..08fc1f12d4 100644 --- a/contrib/plugins/execlog.c +++ b/contrib/plugins/execlog.c @@ -232,12 +232,15 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb) */ if (disas_assist && rmatches) { check_regs_next = false; - gchar *args = g_strstr_len(insn_disas, -1, " "); - for (int n = 0; n < all_reg_names->len; n++) { - gchar *reg = g_ptr_array_index(all_reg_names, n); - if (g_strrstr(args, reg)) { - check_regs_next = true; - skip = false; + const gchar *args = strpbrk(insn_disas, " \t");We have a general preference for glib here, could we use g_strsplit_set? Something like: modified contrib/plugins/execlog.c @@ -232,12 +232,14 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb) */ if (disas_assist && rmatches) { check_regs_next = false; - gchar *args = g_strstr_len(insn_disas, -1, " "); - for (int n = 0; n < all_reg_names->len; n++) { - gchar *reg = g_ptr_array_index(all_reg_names, n); - if (g_strrstr(args, reg)) { - check_regs_next = true; - skip = false; + g_auto(GStrv) args = g_strsplit_set(insn_disas, " \t", 2); + if (args && args[1]) { + for (int n = 0; n < all_reg_names->len; n++) { + gchar *reg = g_ptr_array_index(all_reg_names, n); + if (g_strrstr(args[1], reg)) { + check_regs_next = true; + skip = false; + }
Certainly, and thanks for the suggestion! May I credit you with a "Suggested-by" or "Co-authored-by" tag in v2 of the patch?
+ if (args) { + for (int n = 0; n < all_reg_names->len; n++) { + const gchar *reg = g_ptr_array_index(all_reg_names, n); + if (g_strrstr(args, reg)) { + check_regs_next = true; + skip = false; + break; + } } } }
