10.0-RC1: net/mpd5 crashes in NgMkSockNode due to stack alignment on ARM EABI

[John-Mark Gurney]

Guy Yur wrote this message on Sun, Dec 22, 2013 at 00:46 +0200:
> On Sat, Dec 21, 2013 at 9:15 PM, John-Mark Gurney <jmg at funkthat.com> wrote:
> > Guy Yur wrote this message on Sat, Dec 21, 2013 at 19:24 +0200:
> >> I am running 10.0-RC1 on the BeagleBone Black and the net/mpd5 port is
> >> crashing in libnetgraph NgMkSockNode due to stack alignment.
> >>
> >> 10.0-RC1 World and kernel were compiled in a VirtualBox VM running
> >> 9.2-RELEASE-p2 i386.
> >> clang and ARM_EABI used as the default make options.
> >>
> >> Added prints in NgMkSockNode show rbuf is aligned on 2-byte and not
> >> 4-byte which is needed to access ni->id (a uint32_t).
> >>
> >> ni = 0xbfffe87a
> >> rbuf = 0xbfffe842
> >> sizeof(resp->header) = 56
> >>
> >>
> >> (gdb) bt
> >> #0  0x201529a0 in NgMkSockNode (name=<value optimized out>, csp=0xbfffe95c,
> >>     dsp=0xbfffe958) at /usr/src/lib/libnetgraph/sock.c:134
> >> #1  0x00037b9c in MppcTestCap () at ccp_mppc.c:754
> >> #2  0x0007c1f4 in main (ac=4, av=0xbfffeb90) at main.c:248
> >> #3  0x0000d1b0 in __start (argc=4, argv=0xbfffeb90, env=0xbfffeba4,
> >>     ps_strings=<value optimized out>, obj=<value optimized out>,
> >>     cleanup=<value optimized out>) at /usr/src/lib/csu/arm/crt1.c:115
> >> #4  0x203e9dc0 in _thr_ast (curthread=0x200fd000)
> >>     at /usr/src/lib/libthr/thread/thr_sig.c:265
> >>
> >>
> >> Putting rbuf in a union with struct ng_mesg sorted the alignment to
> >> 4-byte and mpd5 didn't crash.
> >> I attached the changes I used to test mpd5 doesn't crash with correct alignment.
> >
> > The patch looks correct, but lets make sure that the -net people don't
> > have an issue with it...
> >
> > I've reattached Guy's patch for review.
> >
> > Guy, bug me in a week or so if I haven't committed it, and I will...
> 
> Should I still file a PR?

Yes, please.  It provides a link from the mailing list to the commit and
provides historical reference...

> 1. I noticed my patch causes a style bug with the rbuf line now taking
> 87 columns.
> 2. Since the union now has a ng_mesg struct, the casting to resp can
> be skipped and the union member used directly.

Thanks for these.

> Attached new patch breaking the rbuf line and swapping resp usage with
> res.res and &res.res
> Maybe a different name than res should be used for the union or the member.

Maybe, but at the same time, since you define the union locally, the
reader shouldn't be TOO confused...  :)

> I only tested the new patch compiles for arm.armv6, haven't verified it.

Can you please verify it?  Even though I agree you didn't change anything
w/ the new patch, you'd be surprised what computers think. :)

Thanks.

-- 
  John-Mark Gurney				Voice: +1 415 225 5579

     "All that I will do, has been done, All that I have, has not."