Yodel Eldar <yodel.el...@gmail.com> writes:
> 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?
Suggested-by is fine ;-)
>
>
>>> + 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;
>>> + }
>>> }
>>> }
>>> }
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
