Re: [PATCH v2 3/5] Expand 'fork_inferior' to check whether 'traceme_fun' succeeded

Pedro Alves
2020-03-27T13:06:14Z

On 3/17/20 3:47 PM, Sergio Durigan Junior via Gdb-patches wrote:
> This patch is one important piece of the series. It expands
> 'fork_inferior' in order to deal with new steps in the process of
> initializing the inferior. We now have to:
>
> - Create a pipe that will be used to communicate with our
> fork (pre-exec), and which the fork will use to pass back to us the
> errno value of the 'traceme_fun' call.
>
> - Close this pipe after it is used.
>
> - Check the errno value passed back from the fork, and report any
> problems in the initialization to the user.
>
> I thought about and implemented a few designs for all of this, but
> ended up sticking with the function overload one. 'fork_inferior' is
> now two functions: one that will take a traceme function like
> '(*traceme_fun) ()' --- i.e., the original 'fork_inferior' behaviour,
> and other that will take a function like '(*traceme_fun) (int
> trace_pipe_write)'. Depending on which function it takes, we know
> whether the user does not want us to check whether the 'traceme_fun'
> call was successful (former) or if she does (latter).

Pedantically: user -> caller. she -> it. "user" normally refers to
a person who uses gdb.

> All in all, the patch is not complicated to understand and keeps the
> interface clean enough so that we don't have to update every caller of
> 'fork_inferior' (which was a problem with previous designs I tried).
>
> The subsequent patch will build on top of this one and implement the
> errno-passing-via-pipe on the GNU/Linux target.
>
> gdb/ChangeLog:
> yyyy-mm-dd Sergio Durigan Junior <sergiodj@redhat.com>
>
> * nat/fork-inferior.c: Include "gdbsupport/scoped_pipe.h".
> (default_trace_me_fail_reason): New function.
> (trace_me_fail_reason): New variable.
> (write_trace_errno_to_pipe): New function.
> (read_trace_errno_from_pipe): Likewise.
> (check_child_trace_me_errno): Likewise.
> (traceme_info): New struct.
> (fork_inferior_1): Renamed from 'fork_inferior'.
> (fork_inferior): New overloads.
> (trace_start_error_with_name): Add "append" parameter.
> * nat/fork-inferior.h (fork_inferior): Expand comment.
> Add overload declaration.
> (trace_start_error_with_name): Add "append" parameter.
> (trace_me_fail_reason): New variable.
> (check_child_trace_me_errno): New function.
> (write_trace_errno_to_pipe): Likewise.
> ---
> gdb/nat/fork-inferior.c | 231 ++++++++++++++++++++++++++++++++++++----
> gdb/nat/fork-inferior.h | 87 ++++++++++++---
> 2 files changed, 288 insertions(+), 30 deletions(-)
>
> diff --git a/gdb/nat/fork-inferior.c b/gdb/nat/fork-inferior.c
> index 1185ef8998..223ff44195 100644
> --- a/gdb/nat/fork-inferior.c
> +++ b/gdb/nat/fork-inferior.c
> @@ -27,6 +27,7 @@
> #include "gdbsupport/pathstuff.h"
> #include "gdbsupport/signals-state-save-restore.h"
> #include "gdbsupport/gdb_tilde_expand.h"
> +#include "gdbsupport/scoped_pipe.h"
> #include <vector>
>
> extern char **environ;
> @@ -262,16 +263,157 @@ execv_argv::init_for_shell (const char *exec_file,
> m_argv.push_back (NULL);
> }
>
> -/* See nat/fork-inferior.h. */
> +/* Default implementation of 'trace_me_fail_reason'. Always return
> + an empty string. */
>
> -pid_t
> -fork_inferior (const char *exec_file_arg, const std::string &allargs,
> - char **env, void (*traceme_fun) (),
> - gdb::function_view<void (int)> init_trace_fun,
> - void (*pre_trace_fun) (),
> - const char *shell_file_arg,
> - void (*exec_fun)(const char *file, char * const *argv,
> - char * const *env))
> +static std::string
> +default_trace_me_fail_reason (int err)
> +{
> + return {};
> +}
> +
> +/* See fork-inferior.h. */
> +
> +std::string (*trace_me_fail_reason) (int err)
> + = default_trace_me_fail_reason;
> +
> +/* See fork-inferior.h. */
> +
> +void
> +write_trace_errno_to_pipe (int writepipe, int trace_errno)
> +{
> + ssize_t writeret;
> +
> + do
> + {
> + writeret = write (writepipe, &trace_errno, sizeof (trace_errno));
> + }
> + while (writeret < 0 && (errno == EAGAIN || errno == EINTR));
> +
> + if (writeret < 0)
> + error (_("Could not write trace errno: %s"), safe_strerror (errno));

I'll have a comment about this error in a following patch.

> +}
> +
> +/* Helper function to read TRACE_ERRNO from READPIPE, which handles
> + EINTR/EAGAIN and throws and exception if there was an error. */
> +
> +static int
> +read_trace_errno_from_pipe (int readpipe)
> +{
> + ssize_t readret;
> + int trace_errno;
> +
> + do
> + {
> + readret = read (readpipe, &trace_errno, sizeof (trace_errno));
> + }
> + while (readret < 0 && (errno == EAGAIN || errno == EINTR));
> +
> + if (readret < 0)
> + error (_("Could not read trace errno: %s"), safe_strerror (errno));
> +
> + return trace_errno;
> +}
> +
> +/* See fork-inferior.h. */
> +
> +void
> +check_child_trace_me_errno (int readpipe)
> +{
> + fd_set rset;
> + struct timeval timeout;
> + int ret;
> +
> + /* Make sure we have a valid 'trace_me_fail_reason' function
> + defined. */
> + gdb_assert (trace_me_fail_reason != nullptr);
> +
> + FD_ZERO (&rset);
> + FD_SET (readpipe, &rset);
> +
> + /* Five seconds should be plenty of time to wait for the child's
> + reply. */
> + timeout.tv_sec = 5;
> + timeout.tv_usec = 0;
> +
> + do
> + {
> + ret = select (readpipe + 1, &rset, NULL, NULL, &timeout);
> + }
> + while (ret < 0 && (errno == EAGAIN || errno == EINTR));
> +
> + if (ret < 0)
> + perror_with_name ("select");
> + else if (ret == 0)
> + error (_("Timeout while waiting for child's trace errno"));
> + else
> + {
> + int child_errno;
> +
> + child_errno = read_trace_errno_from_pipe (readpipe);
> +
> + if (child_errno != 0)
> + {
> + /* The child can't use TRACE_TRACEME. We have to check whether
> + we know the reason for the failure, and then error out. */
> + std::string reason = trace_me_fail_reason (child_errno);
> +
> + if (reason.empty ())
> + reason = "Could not determine reason for trace failure.";

Missing i18n.

> +
> + /* The child is supposed to display a warning containing the
> + safe_strerror message before us, so we just display the
> + possible reason for the failure. */
> + error ("%s", reason.c_str ());
> + }
> + }
> +}
> +
> +/* Helper struct for fork_inferior_1, containing information on
> + whether we should check if TRACEME_FUN was successfully called or
> + not. */

"was successfully called" seems ambiguous -- I first read it as,
"whether we managed to call TRACEME_FUN", which is not what you meant,
since we always call it.

It'd suggest:

"should check whether TRACEME_FUN succeeded tracing the child"

> +
> +struct traceme_info
> +{
> + /* True if we should check whether the call to 'traceme_fun
> + (TRACE_ME...)' succeeded or not. */
> + bool check_trace_me_fail_reason;
> +
> + union
> + {
> + /* The non-check version of TRACEME_FUN. It will be set if
> + CHECK_TRACEME_FAIL_REASON is false.
> +
> + This function will usually just perform the call to whatever
> + trace function needed to start tracing the inferior (e.g.,
> + ptrace). */
> + void (*traceme_fun_nocheck) ();
> +
> + /* The check version of TRACEME_FUN. It will be set if
> + CHECK_TRACEME_FAIL_REASON is true.
> +
> + This function will usually perform the call to whatever trace
> + function needed to start tracing the inferior, but will also
> + write its errno value to TRACE_ERRNO_PIPE, so that
> + fork_inferior_1 can check whether it suceeded. */

Typo: "suceeded" -> "succeeded".

I'd drop the "usually" both places. It's not usually, it's always.

> + void (*traceme_fun_check) (int trace_errno_pipe);
> + } u;
> +};
> +
> +/* Helper function.
> +
> + Depending on the value of TRACEME_INFO.CHECK_TRACEME_FAIL_REASON,
> + this function will check whether the call to TRACEME_FUN succeeded
> + or not. */
> +
> +static pid_t
> +fork_inferior_1 (const char *exec_file_arg, const std::string &allargs,
> + char **env, const struct traceme_info traceme_info,
> + gdb::function_view<void (int)> init_trace_fun,
> + void (*pre_trace_fun) (),
> + const char *shell_file_arg,
> + void (*exec_fun)(const char *file, char * const *argv,
> + char * const *env))
> {
> pid_t pid;
> /* Set debug_fork then attach to the child while it sleeps, to debug. */
> @@ -283,6 +425,7 @@ fork_inferior (const char *exec_file_arg, const std::string &allargs,
> int save_errno;
> const char *inferior_cwd;
> std::string expanded_inferior_cwd;
> + scoped_pipe trace_pipe;

You could wrap this in a gdb::optional to avoid creating the pipe
unless necessary. Like:

gdb::optional<scoped_pipe> trace_pipe;
if (traceme_info.check_trace_me_fail_reason)
trace_pipe.emplace ();

>
> /* If no exec file handed to us, get it from the exec-file command
> -- with a good, common error message if none is specified. */
> @@ -365,12 +508,6 @@ fork_inferior (const char *exec_file_arg, const std::string &allargs,
>
> if (pid == 0)
> {
> - /* Close all file descriptors except those that gdb inherited
> - (usually 0/1/2), so they don't leak to the inferior. Note
> - that this closes the file descriptors of all secondary
> - UIs. */
> - close_most_fds ();
> -
> /* Change to the requested working directory if the user
> requested it. */
> if (inferior_cwd != NULL)
> @@ -392,7 +529,10 @@ fork_inferior (const char *exec_file_arg, const std::string &allargs,
> for the inferior. */
>
> /* "Trace me, Dr. Memory!" */
> - (*traceme_fun) ();
> + if (traceme_info.check_trace_me_fail_reason)
> + (*traceme_info.u.traceme_fun_check) (trace_pipe.get_write_end ());
> + else
> + (*traceme_info.u.traceme_fun_nocheck) ();
>
> /* The call above set this process (the "child") as debuggable
> by the original gdb process (the "parent"). Since processes
> @@ -403,6 +543,12 @@ fork_inferior (const char *exec_file_arg, const std::string &allargs,
> saying "not parent". Sorry; you'll have to use print
> statements! */
>
> + /* Close all file descriptors except those that gdb inherited
> + (usually 0/1/2), so they don't leak to the inferior. Note
> + that this closes the file descriptors of all secondary
> + UIs, and the trace errno pipe (if it's open). */
> + close_most_fds ();
> +
> restore_original_signals_state ();
>
> /* There is no execlpe call, so we have to set the environment
> @@ -431,6 +577,13 @@ fork_inferior (const char *exec_file_arg, const std::string &allargs,
> _exit (0177);
> }
>
> + if (traceme_info.check_trace_me_fail_reason)
> + {
> + /* Check the trace errno, and inform the user about the reason
> + of the failure, if there was any. */
> + check_child_trace_me_errno (trace_pipe.get_read_end ());
> + }
> +

check_child_trace_me_errno can throw an error. So it should be done after
we've restored all global state. See the environ restore just below ...

> /* Restore our environment in case a vforked child clob'd it. */
> environ = save_our_env;

... here. You'd miss restoring this.

>
> @@ -448,6 +601,48 @@ fork_inferior (const char *exec_file_arg, const std::string &allargs,
> return pid;
> }
>
> +/* See fork-inferior.h. */
> +
> +pid_t
> +fork_inferior (const char *exec_file_arg, const std::string &allargs,
> + char **env, void (*traceme_fun) (),
> + gdb::function_view<void (int)> init_trace_fun,
> + void (*pre_trace_fun) (),
> + const char *shell_file_arg,
> + void (*exec_fun)(const char *file, char * const *argv,

Something odd with indentation here. Likely tabs vs spaces.

> + char * const *env))
> +{
> + struct traceme_info traceme_info;
> +
> + traceme_info.check_trace_me_fail_reason = false;
> + traceme_info.u.traceme_fun_nocheck = traceme_fun;
> +
> + return fork_inferior_1 (exec_file_arg, allargs, env, traceme_info,
> + init_trace_fun, pre_trace_fun, shell_file_arg,
> + exec_fun);
> +}
> +
> +/* See fork-inferior.h. */
> +
> +pid_t
> +fork_inferior (const char *exec_file_arg, const std::string &allargs,
> + char **env, void (*traceme_fun) (int trace_errno_pipe),
> + gdb::function_view<void (int)> init_trace_fun,
> + void (*pre_trace_fun) (),
> + const char *shell_file_arg,
> + void (*exec_fun)(const char *file, char * const *argv,

Ditto re. indentation.

> + char * const *env))
> +{
> + struct traceme_info traceme_info;
> +
> + traceme_info.check_trace_me_fail_reason = true;
> + traceme_info.u.traceme_fun_check = traceme_fun;
> +
> + return fork_inferior_1 (exec_file_arg, allargs, env, traceme_info,
> + init_trace_fun, pre_trace_fun, shell_file_arg,
> + exec_fun);
> +}
> +
> /* See nat/fork-inferior.h. */
>
> ptid_t
> @@ -592,7 +787,7 @@ trace_start_error (const char *fmt, ...)
> /* See nat/fork-inferior.h. */
>
> void
> -trace_start_error_with_name (const char *string)
> +trace_start_error_with_name (const char *string, const char *append)
> {
> - trace_start_error ("%s: %s", string, safe_strerror (errno));
> + trace_start_error ("%s: %s%s", string, safe_strerror (errno), append);
> }
> diff --git a/gdb/nat/fork-inferior.h b/gdb/nat/fork-inferior.h
> index cf6f137edd..b67215353f 100644
> --- a/gdb/nat/fork-inferior.h
> +++ b/gdb/nat/fork-inferior.h
> @@ -32,17 +32,41 @@ struct process_stratum_target;
> #define START_INFERIOR_TRAPS_EXPECTED 1
>
> /* Start an inferior Unix child process and sets inferior_ptid to its
> - pid. EXEC_FILE is the file to run. ALLARGS is a string containing
> - the arguments to the program. ENV is the environment vector to
> - pass. SHELL_FILE is the shell file, or NULL if we should pick
> - one. EXEC_FUN is the exec(2) function to use, or NULL for the default
> - one. */
> -
> -/* This function is NOT reentrant. Some of the variables have been
> - made static to ensure that they survive the vfork call. */
> + pid.
> +
> + EXEC_FILE is the file to run.
> +
> + ALLARGS is a string containing the arguments to the program.
> +
> + ENV is the environment vector to pass.
> +
> + SHELL_FILE is the shell file, or NULL if we should pick one.
> +
> + EXEC_FUN is the exec(2) function to use, or NULL for the default
> + one.
> +
> + This function is NOT reentrant. Some of the variables have been
> + made static to ensure that they survive the vfork call.
> +
> + This function does not check whether the call to TRACEME_FUN
> + succeeded or not. */
> extern pid_t fork_inferior (const char *exec_file_arg,
> const std::string &allargs,
> - char **env, void (*traceme_fun) (),
> + char **env,
> + void (*traceme_fun) (),
> + gdb::function_view<void (int)> init_trace_fun,
> + void (*pre_trace_fun) (),
> + const char *shell_file_arg,
> + void (*exec_fun) (const char *file,
> + char * const *argv,
> + char * const *env));
> +
> +/* Like fork_inferior above, but check whether the call to TRACEME_FUN
> + succeeded or not. */
> +extern pid_t fork_inferior (const char *exec_file_arg,
> + const std::string &allargs,
> + char **env,
> + void (*traceme_fun) (int trace_errno_pipe),
> gdb::function_view<void (int)> init_trace_fun,
> void (*pre_trace_fun) (),
> const char *shell_file_arg,
> @@ -82,9 +106,48 @@ extern void trace_start_error (const char *fmt, ...)
> ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 2);
>
> /* Like "trace_start_error", but the error message is constructed by
> - combining STRING with the system error message for errno. This
> - function does not return. */
> -extern void trace_start_error_with_name (const char *string)
> + combining STRING with the system error message for errno, and
> + (optionally) with APPEND. This function does not return. */
> +extern void trace_start_error_with_name (const char *string,
> + const char *append = "")
> ATTRIBUTE_NORETURN;
>
> +/* Pointer to function which can be called by
> + 'check_child_trace_me_errno' when we need to determine the reason
> + of a e.g. 'ptrace (PTRACE_ME, ...)' failure. ERR is the ERRNO
> + value set by the failing ptrace call.
> +
> + By default, the function returns an empty string (see
> + fork-inferior.c).
> +
> + This pointer can be overriden by targets that want to personalize
> + the error message printed when trace fails (see linux-nat.c or
> + gdbserver/linux-low.c, for example). */
> +extern std::string (*trace_me_fail_reason) (int err);
> +
> +/* Check the "trace me" errno (generated when executing e.g. 'ptrace
> + (PTRACE_ME, ...)') of the child process that was created by
> + GDB/GDBserver when creating an inferior. The errno value will be
> + passed via a pipe (see 'fork_inferior'), and READPIPE is the read
> + end of the pipe.
> +
> + If possible (i.e., if 'trace_me_fail_reason' is defined by the
> + target), then we also try to determine the possible reason for a
> + failure.
> +
> + The idea is to wait a few seconds (via 'select') until something is
> + written on READPIPE. When that happens, we check if the child's
> + trace errno is different than 0. If it is, we call the function
> + 'trace_me_fail_reason' and try to obtain the reason for the
> + failure, and then throw an exception (with the reason as the
> + exception's message).
> +
> + If nothing is written on the pipe, or if 'select' fails, we also
> + throw exceptions. */
> +extern void check_child_trace_me_errno (int readpipe);
> +
> +/* Helper function to write TRACE_ERRNO to WRITEPIPE, which handles
> + EINTR/EAGAIN and throws an exception if there was an error. */
> +extern void write_trace_errno_to_pipe (int writepipe, int trace_errno);
> +
> #endif /* NAT_FORK_INFERIOR_H */
>

Thanks,
Pedro Alves