Re: [PATCH] set saner MAX_ARGS

[Denys V. <dvl...@re...>]

On Fri, 2011-08-19 at 00:02 +0400, Dmitry V. Levin wrote:
> On Thu, Aug 18, 2011 at 12:47:38PM +0200, Denys Vlasenko wrote:
> > On Thu, 2011-08-18 at 12:23 +0200, Denys Vlasenko wrote:
> > > I noticed that tcp->u_args[MAX_ARGS] array is way larger than
> > > I'd expect: for all arches except HPPA it has 32 (!) elements.
> > > 
> > > I looked at the code and so far I spotted only one abuser of
> > > this fact: sys_sigreturn. On several arches, it saves sigset_t
> > > into tcp->u_args[1...N] on entry and prints it on exit, a-la
> > > 
> > >     memcpy(&tcp->u_arg[1], &sc.oldmask[0], sizeof(sigset_t))
> > > 
> > > The problem here is that in glibc sigset_t is insanely large:
> > > 128 bytes, and using sizeof(sigset_t) in memcpy will overrun
> > > &tcp->u_args[1] even with MAX_ARGS == 32:
> > > On 32 bits, sizeof(tcp->u_args) == 32*4 == 128 bytes!
> > > We may already have a bug there!
> > > 
> > > I propose to change the code to save NSIG / 8 bytes only.
> > > NSIG can't ever be > 256, and in practice is <= 129,
> > > thus NSIG / 8 is <= 16 bytes == 4 32-bit words,
> > > and MAX_ARGS == 5 should be enough for saving signal masks.
> 
> I agree.
> 
> > > The proposed patch is below.
> > > 
> > > Alternative solution is to make sys_sigreturn print mask
> > > on entry, not on exit. What is the reson it doesn't do that now?
> 
> Can sys_sigreturn be interrupted somehow?

Does it matter?


Currently, sigreturn in strace looks like this:

wait4(-1, 0xbfa81764, WSTOPPED, NULL)   = ? ERESTARTSYS (To be restarted)
--- {si_signo=SIGINT, si_code=SI_USER, si_pid=13194, si_uid=0} (Interrupt) ---
sigreturn()                             = ? (mask now [])
--- {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=13194, si_status=0, si_utime=0, si_stime=0} (Child exited) ---
sigreturn()                             = ? (mask now [])
wait4(-1, [{WIFEXITED(s) && WEXITSTATUS(s) == 0}], WSTOPPED, NULL) = 13194


I propose to change it into something like this:

wait4(-1, 0xbfa81764, WSTOPPED, NULL)   = ? ERESTARTSYS (To be restarted)
--- {si_signo=SIGINT, si_code=SI_USER, si_pid=13194, si_uid=0} (Interrupt) ---
sigreturn([])                           = 0
--- {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=13194, si_status=0, si_utime=0, si_stime=0} (Child exited) ---
sigreturn([])                           = 0
wait4(-1, [{WIFEXITED(s) && WEXITSTATUS(s) == 0}], WSTOPPED, NULL) = 13194

because that's how it _really_ happens: the signal mask is actually known
BEFORE sigreturn is executed, so why are we printing it AFTER it returns?
(And in the process, torture ourselves with stupid saving/restoring of
sigmask for that). Look at sys_sigreturn() in strace source to see it.

(The only wart here is that sigmask is not a sigreturn's parameter, so
printing it as "sigreturn([])" is slightly wrong/misleading. Any ideas
how to show it better? With curly braces maybe - {[mask]}?)

Do you like this idea?


> > --- strace.5/defs.h	2011-08-18 11:57:30.512416447 +0200
> > +++ strace.6/defs.h	2011-08-18 11:46:56.349540479 +0200
> > @@ -64,7 +64,7 @@
> >  #define DEFAULT_ACOLUMN	40	/* default alignment column for results */
> >  #endif
> >  #ifndef MAX_ARGS
> > -# ifdef HPPA
> > +# if defined HPPA || defined X86_64 || defined I386
> >  #  define MAX_ARGS	6	/* maximum number of args to a syscall */
> >  # else
> >  /* Way too big. Switch your arch to saner size after you tested that it works */
> 
> What about other architectures?  Is there any with MAX_ARGS > 6?
> Can we assume MAX_ARGS == 6 on linux?

I looked deeper. Apparently FREEBSD needs MAX_ARGS = 8. Also there's
a bug in sys_mmap64 where it will try to access (but not write to)
u_args[6,7] wrongly - see other mail.

My current knowledge is summed up by this comment:

/* Maximum number of args to a syscall.
 *
 * Make sure that all entries in all syscallent.h files
 * have nargs <= MAX_ARGS!
 * linux/<ARCH>/syscallent.h: ia64 has many syscalls with
 * nargs = 8, mips has two with nargs = 7 (both are printargs),
 * all others are <= 6.
 * freebsd/i386/syscallent.h: one syscall with nargs = 8
 * (sys_sendfile, looks legitimate)
 * and one with nargs = 7 (sys_mmap, maybe it should have 6?).
 * sunos4/syscallent.h: all are <= 6.
 * svr4/syscallent.h: all are -1.
 */

Basically, all linux arches sans ia64 should be ok with MAX_ARGS = 6.

Looks like mips needs fixing - only lone two printargs with nargs = 7??
FREEBSD sys_mmap's nargs look wrong too.

For now, I plan to commit the updated patch which sets MAX_ARGS
to 6 (or 8 for FREEBSD) on X86_64 & I386 only.

-- 
vda