Re: [PULL 12/29] gdbstub: Implement follow-fork-mode child

[Peter Maydell <peter.maydell@linaro.org>]

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