Date: Sun, 27 Oct 2013 22:04:06 -0700 From: Adam Leventhal <ahl@delphix.com> To: Mark Johnston <markj@freebsd.org> Cc: dtrace@freebsd.org Subject: Re: Firefox crash during dtrace attach under -CURRENT Message-ID: <CALWP=0bkfzn9_9GsHiGzfY79RsvTyCMfDHkTe%2ByEaahXhHz0nQ@mail.gmail.com> In-Reply-To: <20131024025902.GA2286@charmander> References: <20131023203009.GA92945@lemon> <20131024025902.GA2286@charmander>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CALWP=0bkfzn9_9GsHiGzfY79RsvTyCMfDHkTe%2ByEaahXhHz0nQ>