[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PULL 12/29] gdbstub: Implement follow-fork-mode child
From: |
Peter Maydell |
Subject: |
Re: [PULL 12/29] gdbstub: Implement follow-fork-mode child |
Date: |
Mon, 11 Mar 2024 11:48:11 +0000 |
On Wed, 6 Mar 2024 at 14:42, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> From: Ilya Leoshkevich <iii@linux.ibm.com>
>
> Currently it's not possible to use gdbstub for debugging linux-user
> code that runs in a forked child, which is normally done using the `set
> follow-fork-mode child` GDB command. Purely on the protocol level, the
> missing piece is the fork-events feature.
>
> However, a deeper problem is supporting $Hg switching between different
> processes - right now it can do only threads. Implementing this for the
> general case would be quite complicated, but, fortunately, for the
> follow-fork-mode case there are a few factors that greatly simplify
> things: fork() happens in the exclusive section, there are only two
> processes involved, and before one of them is resumed, the second one
> is detached.
>
> This makes it possible to implement a simplified scheme: the parent and
> the child share the gdbserver socket, it's used only by one of them at
> any given time, which is coordinated through a separate socketpair. The
> processes can read from the gdbserver socket only one byte at a time,
> which is not great for performance, but, fortunately, the
> follow-fork-mode handling involves only a few messages.
>
> Advertise the fork-events support, and remember whether GDB has it
> as well. Implement the state machine that is initialized on fork(),
> decides the current owner of the gdbserver socket, and is terminated
> when one of the two processes is detached. The logic for the parent and
> the child is the same, only the initial state is different.
>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> Message-Id: <20240219141628.246823-12-iii@linux.ibm.com>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Message-Id: <20240305121005.3528075-13-alex.bennee@linaro.org>
>
Hi; Coverity points out an issue with this code (CID 1539966):
> @@ -376,23 +447,160 @@ static void disable_gdbstub(CPUState *thread_cpu)
>
> void gdbserver_fork_end(CPUState *cpu, pid_t pid)
> {
> + gdbserver_state.state = RS_IDLE;
> + gdbserver_state.allow_stop_reply = false;
> + gdbserver_user_state.running_state = 0;
> + for (;;) {
> + switch (gdbserver_user_state.fork_state) {
> + case GDB_FORK_ENABLED:
> + if (gdbserver_user_state.running_state) {
> + return;
> + }
> + QEMU_FALLTHROUGH;
> + case GDB_FORK_ACTIVE:
> + if (read(gdbserver_user_state.fd, &b, 1) != 1) {
> + goto fail;
> + }
> + gdb_read_byte(b);
> + break;
> + case GDB_FORK_DEACTIVATING:
> + b = GDB_FORK_ACTIVATE;
> + if (write(fd, &b, 1) != 1) {
> + goto fail;
> + }
> + gdbserver_user_state.fork_state = GDB_FORK_INACTIVE;
> + break;
> + case GDB_FORK_INACTIVE:
> + if (read(fd, &b, 1) != 1) {
> + goto fail;
> + }
> + switch (b) {
> + case GDB_FORK_ACTIVATE:
> + gdbserver_user_state.fork_state = GDB_FORK_ACTIVE;
> + break;
> + case GDB_FORK_ENABLE:
> + close(fd);
> + gdbserver_user_state.fork_state = GDB_FORK_ENABLED;
> + break;
In this branch of the switch we close(fd), and then break...
> + case GDB_FORK_DISABLE:
> + gdbserver_user_state.fork_state = GDB_FORK_DISABLED;
> + break;
> + default:
> + g_assert_not_reached();
> + }
> + break;
...and break again, so we leave the for() loop...
> + case GDB_FORK_ENABLING:
> + b = GDB_FORK_DISABLE;
> + if (write(fd, &b, 1) != 1) {
> + goto fail;
> + }
> + close(fd);
> + gdbserver_user_state.fork_state = GDB_FORK_ENABLED;
> + break;
> + case GDB_FORK_DISABLING:
> + b = GDB_FORK_ENABLE;
> + if (write(fd, &b, 1) != 1) {
> + goto fail;
> + }
> + gdbserver_user_state.fork_state = GDB_FORK_DISABLED;
> + break;
> + case GDB_FORK_DISABLED:
> + close(fd);
> + disable_gdbstub(cpu);
> + return;
> + default:
> + g_assert_not_reached();
> + }
> + }
...but at the end of the for loop we will fall into this code:
> +
> +fail:
> + close(fd);
...which tries to close(fd) again, which isn't valid.
> + if (pid == 0) {
> + disable_gdbstub(cpu);
> + }
> }
thanks
-- PMM
- [PULL for 9.0 00/29] maintainer updates (tests, gdbstub, plugins), Alex Bennée, 2024/03/06
- [PULL 05/29] gdbstub: Introduce gdbserver_fork_start(), Alex Bennée, 2024/03/06
- [PULL 02/29] gdbstub: Support disablement in a multi-threaded process, Alex Bennée, 2024/03/06
- [PULL 10/29] gdbstub: Introduce gdb_handle_set_thread_user(), Alex Bennée, 2024/03/06
- [PULL 04/29] {linux,bsd}-user: Update ts_tid after fork(), Alex Bennée, 2024/03/06
- [PULL 06/29] {linux,bsd}-user: Pass pid to fork_end(), Alex Bennée, 2024/03/06
- [PULL 09/29] gdbstub: Introduce gdb_handle_query_supported_user(), Alex Bennée, 2024/03/06
- [PULL 08/29] gdbstub: Call gdbserver_fork() both in parent and in child, Alex Bennée, 2024/03/06
- [PULL 12/29] gdbstub: Implement follow-fork-mode child, Alex Bennée, 2024/03/06
- Re: [PULL 12/29] gdbstub: Implement follow-fork-mode child,
Peter Maydell <=
- [PULL 11/29] gdbstub: Introduce gdb_handle_detach_user(), Alex Bennée, 2024/03/06
- [PULL 07/29] {linux,bsd}-user: Pass pid to gdbserver_fork(), Alex Bennée, 2024/03/06
- [PULL 16/29] plugins: implement inline operation relative to cpu_index, Alex Bennée, 2024/03/06
- [PULL 03/29] {linux,bsd}-user: Introduce get_task_state(), Alex Bennée, 2024/03/06
- [PULL 01/29] tests: bump QOS_PATH_MAX_ELEMENT_SIZE again, Alex Bennée, 2024/03/06
- [PULL 18/29] tests/plugin: add test plugin for inline operations, Alex Bennée, 2024/03/06
- [PULL 19/29] tests/plugin/mem: migrate to new per_vcpu API, Alex Bennée, 2024/03/06
- [PULL 14/29] plugins: scoreboard API, Alex Bennée, 2024/03/06
- [PULL 15/29] plugins: define qemu_plugin_u64, Alex Bennée, 2024/03/06
- [PULL 13/29] tests/tcg: Add two follow-fork-mode tests, Alex Bennée, 2024/03/06