Firefox crash during dtrace attach under -CURRENT

[Adam Leventhal]

Hey Mark,

For reference, here's the relevant code from illumos:

#ifdef __amd64
        if (p->p_model == DATAMODEL_NATIVE) {
#endif
                addr = rp->r_sp - sizeof (uintptr_t);
                ret = fasttrap_sulword((void *)addr, rp->r_fp);
#ifdef __amd64
        } else {
                addr = rp->r_sp - sizeof (uint32_t);
                ret = fasttrap_suword32((void *)addr,
                    (uint32_t)rp->r_fp);
        }
#endif

> For anyone interested, the bug is that fasttrap's ebp push instruction
> emulation code is just wrong: it's supposed to save %rbp at %rsp - 8.
> But instead it tries to save %rsp at %rsp - 8, and also reverses the
> uaddr/kaddr arguments to copyout(), resulting in strange crashes. I
> managed to narrow in on the problem with a test program that prints %rbp
> immediately before and after a tracepoint.

Perhaps rp->r_fp was mistakenly swapped for &rp->r_rsp.

> Can anyone review this diff? I'd like to check it in soon, assuming
> that I haven't also made a mistake somewhere. :)

> diff --git a/sys/cddl/contrib/opensolaris/uts/intel/dtrace/fasttrap_isa.c b/sys/cddl/contrib/opensolaris/uts/intel/dtrace/fasttrap_isa.c
> index 8b5ce9f..bb5c9af 100644
> --- a/sys/cddl/contrib/opensolaris/uts/intel/dtrace/fasttrap_isa.c
> +++ b/sys/cddl/contrib/opensolaris/uts/intel/dtrace/fasttrap_isa.c
> @@ -1399,12 +1399,12 @@ fasttrap_pid_probe(struct reg *rp)
>  #ifdef __amd64
>                 if (p->p_model == DATAMODEL_NATIVE) {
>                         addr = rp->r_rsp - sizeof (uintptr_t);
> -                       ret = fasttrap_sulword((void *)addr, &rp->r_rsp);
> +                       ret = fasttrap_sulword(&rp->r_rbp, (void *)addr);
>                 } else {
>  #endif
>  #ifdef __i386__
>                         addr = rp->r_rsp - sizeof (uint32_t);
> -                       ret = fasttrap_suword32((void *)addr, &rp->r_rsp);
> +                       ret = fasttrap_suword32(&rp->r_rbp, (void *)addr);
>  #endif
>  #ifdef __amd64
>                 }

This looks correct, but I'd suggest you might want to instead change
the code in fasttrap_isa.c to look more like the illumos version, and
change the macro definition:

-#define fasttrap_suword64(_k, _u) copyout((_k), (_u), sizeof(uint64_t))
+#define fasttrap_suword64(_u, _k) copyout((_k), &(_u), sizeof(uint64_t))

Adam

-- 
Adam Leventhal
CTO, Delphix
http://blog.delphix.com/ahl