On Wed, Jul 27, 2016 at 08:41:16PM +0200, Nahim El Atmani wrote: > From: Nahim El Atmani <n...@lse.epita.fr> > > From: Nahim El Atmani <nahim+...@naam.me> > > * defs.h: Add new qualifier and struct fault_opts > * linux/x86_64/fault.h: New file. > (fault_set_sc_err): New function. > (fault_discard_sc): Likewise. > * Makefile.am: Add it. > * syscall.c (reallocate_fault): New function. > (qualify_one): Also extend the size of faults_vec. > (qual_fault): New function. > (qual_signal): Also reallocate faults_vec. > (fault_syscall_enter): New function. > (fault_syscall_exit): Likewise. > (trace_syscall_entering): Discard syscall if needed. > (trace_syscall_exiting): Set syscall's return error value, print when > syscall > has been discarded. > > Signed-off-by: Nahim El Atmani <nahim+...@naam.me> > Reviewed-By: Gabriel Laskar <gabr...@lse.epita.fr> > --- > * The fault injection is currently only available for i386 & x86_64, hence a
Why the fault injection is limited to i386 and x86_64?
The mechanism of setting syscall number and error code is similar on all
architectures.
> new configure flag have been created (--enable-fault-injection) to keep the
> build clean on other architectures.
Please don't make this option configurable. It complicates the code
(== causes mistakes) and gives very little in return.
> * Concerning the struct qual_options, the name 'fault' and short name 'f' are
> not fixed, feel free to comment if something better comes to your mind.
> * Regarding the error number checks in qual_fault(), they're light on purpose
> to allow one to return any kind of return value.
Even if they are light, they should be correct.
> * There is still one TODO left which is supporting the fuzzy approach using
> percentage as an occurrence. It should comes soon since all the option
> parsing is done. On that note fuzzing means we want to be able to reproduce,
> so we have to keep the seed somewhere and take it as an input. I my opinion
> using the environment for this kind of things is better than adding a new
> option, what do you think?
Using environment is OK.
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -659,6 +659,11 @@ EXTRA_DIST = \
> xlat/gen.sh \
> xlate.el
>
> +if ENABLE_FAULT_INJECTION
> +EXTRA_DIST += linux/x86_64/fault.h \
> + linux/i386/fault.h
> +endif
> +
When configured without --enable-fault-injection, this would create
tarball without these fault.h files; such tarball won't support
--enable-fault-injection because of missing files.
> @@ -767,7 +788,6 @@ if test "x$use_libunwind" = xyes; then
> AC_SUBST(libunwind_CPPFLAGS)
> fi
> AM_CONDITIONAL([USE_LIBUNWIND], [test "x$use_libunwind" = xyes])
> -AC_MSG_RESULT([$use_libunwind])
What was wrong with the message?
> if test "$arch" = mips && test "$no_create" != yes; then
> mkdir -p linux/mips
> diff --git a/defs.h b/defs.h
> index 2edf943..d4b0dde 100644
> --- a/defs.h
> +++ b/defs.h
> @@ -363,6 +363,9 @@ struct tcb {
> #define QUAL_SIGNAL 0x010 /* report events with this signal */
> #define QUAL_READ 0x020 /* dump data read on this file descriptor */
> #define QUAL_WRITE 0x040 /* dump data written to this file descriptor */
> +#if ENABLE_FAULT_INJECTION
> +#define QUAL_FAULT 0x080 /* this system call fail on purpose */
> +#endif
Why this new macro is ifdef'ed?
> typedef uint8_t qualbits_t;
>
> #define DEFAULT_QUAL_FLAGS (QUAL_TRACE | QUAL_ABBREV | QUAL_VERBOSE)
> @@ -458,6 +461,28 @@ enum iov_decode {
> IOV_DECODE_NETLINK
> };
>
> +#if ENABLE_FAULT_INJECTION
> +/* Fault injection qualifiers concerning syscalls:
> + * FAULT_ENTER: already discarded, but error is not yet propagated
> + * FAULT_DONE: already discarded and we were using FAULT_AT (prevent
> overflow)
> + * FAULT_AT: discard syscall at the nth time
> + * FAULT_EVERY: discard syscall every nth time
> + * FAULT_FUZZY: discard syscall on a random basis every nth percent of the
> time
> + */
> +#define FAULT_ENTER 1
> +#define FAULT_EVERY (1 << 2)
> +#define FAULT_FUZZY (1 << 3)
> +#define FAULT_AT (1 << 4)
> +#define FAULT_DONE (1 << 5)
I prefer enums in new code. BTW, (1 << 1) is missing.
> +static inline long
> +fault_set_sc_err(struct tcb *tcp, int error)
> +{
> + return ptrace(PTRACE_POKEUSER, tcp->pid,
> + offsetof(struct user, regs.eax),
> + (unsigned long long int)error);
> +}
I'd rather call this function set_error or arch_set_error, opposite to
get_error, and placed it into $arch/get_error.c
It might be a good idea to omit the second argument and just set
the error register from tcp->u_error.
> +static inline long
> +fault_discard_sc(struct tcb *tcp)
> +{
> + return ptrace(PTRACE_POKEUSER, tcp->pid,
> + offsetof(struct user, regs.orig_eax),
> + (unsigned long long int)-1);
> +}
Likewise, arch_set_scno and $arch/get_scno.c
get_error.c and get_scno.c could be renamed if necessary.
> --- a/syscall.c
> +++ b/syscall.c
> @@ -266,6 +266,14 @@ enum {
> MIN_QUALS = MAX_NSYSCALLS > 255 ? MAX_NSYSCALLS : 255
> };
>
> +#if ENABLE_FAULT_INJECTION
> +#include "fault.h"
> +struct fault_opts *faults_vec[SUPPORTED_PERSONALITIES];
> +#define syscall_failed(tcp) \
> + (((tcp)->qual_flg & QUAL_FAULT) && \
> + (faults_vec[current_personality][(tcp)->scno].flag & FAULT_ENTER))
> +#endif
This is wrong: faults_vec[current_personality][(tcp)->scno].flag
is a global flag, it cannot be used as a state of syscall parsing
for a tracee.
Use tcp->flags to store this information.
> + opts.occ = strtol(ss, &end, 10);
Result of strtol is assigned to int.
> + if (end == ss || (*end != '\0' && *end != '%' && *end != '.')
> + || errno == ERANGE || errno == EINVAL || opts.occ < 1
errno is not cleared before the call.
--
ldv
pgpHIhu1JJLPa.pgp
Description: PGP signature
------------------------------------------------------------------------------
_______________________________________________ Strace-devel mailing list Strace-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/strace-devel
