[PATCHv2 1/2] strace: add :signal option to efault
Dmitry V. Levin
ldv at altlinux.org
Wed Dec 28 00:44:02 UTC 2016
On Tue, Dec 27, 2016 at 12:14:06PM +0100, Seraphime Kirkovski wrote:
> This extends the -e fault capability with a :signal option which
> delivers a signal on entry of the specified syscall.
>
> :signal and :error are complementary, if they are both specified the
> syscall will be injected as per normal and a signal will be sent to the
> process.
>
> Signed-off-by: Seraphime Kirkovski <kirkseraph at gmail.com>
Let's call it a signal injection. I suggest some changes to the commit
message, e.g.
Implement signal injection
This extends the fault injection capability with :signal=SIG option
which injects a signal on entering each syscall from the specified set.
:signal and :error options are complementary, if they are both specified
the syscall will be fault injected as usual and the specified signal
will be delivered to the tracee.
> ---
>
> This fixes the problems discussed in the last patch:
>
> * fault_opts.signo becomes unsigned
> * fault.opts.err becomes signed
> * use 0 as default value for signal
> * remove unneccessary casts
> * don't enforce ENOSYS
> * Add a hardlimit for signals - SIGRTMAX
> strace crashes when :signal is greater than the biggest signal
Good point, there is no use to try injecting signals greater than _NSIG.
> defs.h | 6 ++++--
> qualify.c | 12 +++++++++++-
> strace.c | 4 +++-
> syscall.c | 16 +++++++++-------
> 4 files changed, 27 insertions(+), 11 deletions(-)
>
> diff --git a/defs.h b/defs.h
> index 0a4c616..a766808 100644
> --- a/defs.h
> +++ b/defs.h
> @@ -217,7 +217,8 @@ typedef struct ioctlent {
> struct fault_opts {
> uint16_t first;
> uint16_t step;
> - uint16_t err;
> + int16_t err;
> + uint16_t signo;
> };
>
> #if defined LINUX_MIPSN32 || defined X32
> @@ -457,13 +458,14 @@ extern int read_int_from_file(const char *, int *);
> extern void set_sortby(const char *);
> extern void set_overhead(int);
> extern void print_pc(struct tcb *);
> -extern int trace_syscall(struct tcb *);
> +extern int trace_syscall(struct tcb *, unsigned int *);
> extern void count_syscall(struct tcb *, const struct timeval *);
> extern void call_summary(FILE *);
>
> extern void clear_regs(void);
> extern void get_regs(pid_t pid);
> extern int get_scno(struct tcb *tcp);
> +
> /**
> * Convert syscall number to syscall name.
> *
> diff --git a/qualify.c b/qualify.c
> index a7d276c..b698c4f 100644
> --- a/qualify.c
> +++ b/qualify.c
> @@ -26,6 +26,7 @@
> */
>
> #include "defs.h"
> +#include <signal.h>
Let's include the recently added strace header file "nsig.h"
instead of <signal.h> for the reason explained below.
> typedef unsigned int number_slot_t;
> #define BITS_PER_SLOT (sizeof(number_slot_t) * 8)
> @@ -414,6 +415,11 @@ parse_fault_token(const char *const token, struct fault_opts *const fopts)
> if (intval < 1)
> return false;
> fopts->err = intval;
> + } else if ((val = strip_prefix("signal=", token))) {
> + intval = sigstr_to_uint(val);
> + if (intval < 0 || intval > SIGRTMAX)
> + return false;
First, signal 0 is not a valid signal, so the first part of the check
should be intval < 1.
Second, strace does not trust SIGRTMAX provided by libc. It might be good
enough for regular code that uses libc functions, but not for strace. You
can peep into signal.c to get an idea why.
Up to this day, strace used to employ NSIG / 8 in most cases, which was a
bit awkward so I've just created a new header called "nsig.h" with proper
definitions of NSIG and NSIG_BYTES == NSIG / 8.
What I suggest for this case is to replace intval > SIGRTMAX check
with intval > NSIG_BYTES * 8.
> + fopts->signo = intval;
> } else {
> return false;
> }
> @@ -494,13 +500,17 @@ qualify_fault(const char *const str)
> struct fault_opts opts = {
> .first = 1,
> .step = 1,
> - .err = 0
> + .err = -1,
> + .signo = 0
> };
> char *buf = NULL;
> char *name = parse_fault_expression(str, &buf, &opts);
> if (!name) {
> error_msg_and_die("invalid %s '%s'", "fault argument", str);
> }
> + /* If no error, nor signal is specified, fallback to the default platform error */
Please try to avoid writing lines longer than 80 symbols.
In case of multi-line comments, format them this way:
/*
* If neither error nor signal is specified,
* fallback to the default platform error code.
*/
> + if (opts.signo == 0 && opts.err == -1)
> + opts.err = 0;
>
>
> struct number_set tmp_set[SUPPORTED_PERSONALITIES];
> diff --git a/strace.c b/strace.c
> index 4659ddb..23ca108 100644
> --- a/strace.c
> +++ b/strace.c
> @@ -2447,7 +2447,8 @@ show_stopsig:
> * This should be syscall entry or exit.
> * Handle it.
> */
> - if (trace_syscall(tcp) < 0) {
> + sig = 0;
> + if (trace_syscall(tcp, &sig) < 0) {
> /*
> * ptrace() failed in trace_syscall().
> * Likely a result of process disappearing mid-flight.
> @@ -2461,6 +2462,7 @@ show_stopsig:
> */
> return true;
> }
> + goto restart_tracee;
>
> restart_tracee_with_sig_0:
> sig = 0;
> diff --git a/syscall.c b/syscall.c
> index 63d3b00..c86d55c 100644
> --- a/syscall.c
> +++ b/syscall.c
> @@ -573,7 +573,7 @@ tcb_fault_opts(struct tcb *tcp)
>
>
> static long
> -inject_syscall_fault_entering(struct tcb *tcp)
> +inject_syscall_fault_entering(struct tcb *tcp, unsigned int *signo)
> {
> if (!tcp->fault_vec[current_personality]) {
> tcp->fault_vec[current_personality] =
> @@ -595,7 +595,9 @@ inject_syscall_fault_entering(struct tcb *tcp)
>
> opts->first = opts->step;
>
> - if (!arch_set_scno(tcp, -1))
> + if (opts->signo > 0)
> + *signo = opts->signo;
Alternatively, you can omit the check and leave just the assignment:
*signo = opts->signo;
> + if (opts->err != -1 && !arch_set_scno(tcp, -1))
> tcp->flags |= TCB_FAULT_INJ;
>
> return 0;
> @@ -606,7 +608,7 @@ update_syscall_fault_exiting(struct tcb *tcp)
> {
> struct fault_opts *opts = tcb_fault_opts(tcp);
>
> - if (opts && opts->err && tcp->u_error != opts->err) {
> + if (opts && opts->err && tcp->u_error != (uint16_t)opts->err) {
We are not going to get here if opts->err < 0, but let's be defensive
and check for opts->err > 0 instead.
Also, please put a space after the cast: (uint16_t) opts->err.
> unsigned long u_error = tcp->u_error;
> tcp->u_error = opts->err;
> if (arch_set_error(tcp))
> @@ -617,7 +619,7 @@ update_syscall_fault_exiting(struct tcb *tcp)
> }
>
> static int
> -trace_syscall_entering(struct tcb *tcp)
> +trace_syscall_entering(struct tcb *tcp, unsigned int *sig)
> {
> int res, scno_good;
>
> @@ -688,7 +690,7 @@ trace_syscall_entering(struct tcb *tcp)
> }
>
> if (tcp->qual_flg & QUAL_FAULT)
> - inject_syscall_fault_entering(tcp);
> + inject_syscall_fault_entering(tcp, sig);
>
> if (cflag == CFLAG_ONLY_STATS) {
> res = 0;
> @@ -981,10 +983,10 @@ trace_syscall_exiting(struct tcb *tcp)
> }
>
> int
> -trace_syscall(struct tcb *tcp)
> +trace_syscall(struct tcb *tcp, unsigned int *signo)
> {
> return exiting(tcp) ?
> - trace_syscall_exiting(tcp) : trace_syscall_entering(tcp);
> + trace_syscall_exiting(tcp) : trace_syscall_entering(tcp, signo);
> }
>
> bool
--
ldv
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20161228/8b40df39/attachment.bin>
More information about the Strace-devel
mailing list