[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] fix gdbserver_state pointer validation
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [Qemu-devel] [PATCH] fix gdbserver_state pointer validation |
Date: |
Wed, 11 Jul 2018 10:58:28 -0300 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.0 |
Hi Stephane,
On 07/11/2018 04:52 AM, stephane duverger wrote:
> To reach gdb_set_stop_cpu() with gdbserver_state == NULL, you previously
>> entered gdb_vm_state_change() with and use CPUState *cpu =
>> gdbserver_state->c_cpu = NULL deref, which shouldn't happen.
>> Also in gdb_set_stop_cpu() you finally call cpu_single_step(cpu=crap)
>> which then deref crap->singlestep_enabled.
>>
>> So I think this is not the correct fix.
>>
>
> I think you are wrong. You can enter gdb_vm_state_change() only if it has
> been registered through qemu_add_vm_change_state_handler(). And this
> happens in gdbserver_start() which is called only when you start the
> gdbserver stub.
>
> This is exactly what we don't want to do here: use qemu breakpoints and
> debug events forwarding without the need to enable a gdb stub.
Well, at least we agree the gdb stub code is not straightforward.
And apparently without seeing the bigger picture about how you are using
this, I am missing something.
> There might be an historical reason that vCPU debug events are always
> forwarded to the gdbserver (from cpu_handle_guest_debug()) but this
> should not be mandatory.
>
> One could consider a check to the gdbserver state right before:
>
> if (gdbserver_enabled())
> gdb_set_stop_cpu(cpu);
>
> As found in other part of qemu code: kvm_enabled, hax_enabled, ...
>
>
>> Since this shouldn't happen, I'd add an assert(gdbserver_state) in
>> gdb_set_stop_cpu(), and clean gdb_vm_state_change()'s code flow to not
>> reach this.
>>
>
> Correct if you previously add the gdbserver_enabled() check. Else this
> would abort the qemu instance each time a debug event is triggered
> and forwarded to a vm_change_state handler.
>
>>>> void gdb_set_stop_cpu(CPUState *cpu)
>>>>> {
>>>>> + if (!gdbserver_state) {
>>>>> + return;
>>>>> + }
>>>>> gdbserver_state->c_cpu = cpu;
>>>>> gdbserver_state->g_cpu = cpu;
>>
>> I find it safer the opposite way, if (s) { s-> ... }
>>
>
> Sincerely, this argument is subjective. If it's part of Qemu coding
> standard,
> i would agree. Again there is a lot of situations in the Qemu code where
> exit conditions are checked first and then lead to a return, preventing an
> unneeded level of indentation. Seriously, we are talking about a 2 lines
> function.
This is indeed a personal subjective suggestion, not part of QEMU Coding
standard. Rational is, if in the future someone needs to modify
gdb_set_stop_cpu(), it would be easier/safer for him.
No worries ;)
Regards,
Phil.
signature.asc
Description: OpenPGP digital signature