svn commit: r273441 - in head/sys: kern sys

[Mateusz Guzik]

On Wed, Oct 22, 2014 at 11:41:38AM +0300, Konstantin Belousov wrote:
> On Wed, Oct 22, 2014 at 10:26:21AM +0200, Mateusz Guzik wrote:
> > On Wed, Oct 22, 2014 at 10:51:31AM +0300, Konstantin Belousov wrote:
> > > On Wed, Oct 22, 2014 at 12:23:44AM +0000, Mateusz Guzik wrote:
> > > > Author: mjg
> > > > Date: Wed Oct 22 00:23:43 2014
> > > > New Revision: 273441
> > > > URL: https://svnweb.freebsd.org/changeset/base/273441
> > > > 
> > > > Log:
> > > >   filedesc: cleanup setugidsafety a little
> > > >   
> > > >   Rename it to fdsetugidsafety for consistency with other functions.
> > > >   
> > > >   There is no need to take filedesc lock if not closing any files.
> > > >   
> > > >   The loop has to verify each file and we are guaranteed fdtable has space
> > > >   for at least 20 fds. As such there is no need to check fd_lastfile.
> > > ^^^^^^^^^^^^^^^^^^^^^^^^ *
> > > 
> > [..]
> > > >  	fdp = td->td_proc->p_fd;
> > > >  	KASSERT(fdp->fd_refcnt == 1, ("the fdtable should not be shared"));
> > > > -	FILEDESC_XLOCK(fdp);
> > > > -	for (i = 0; i <= fdp->fd_lastfile; i++) {
> > > > -		if (i > 2)
> > > > -			break;
> > > > +	for (i = 0; i <= 2; i++) {
> > > [*] This requires an assert.
> > > 
> > 
> > I was considering adding one, but failed to come up with anything good.
> > 
> > Ideally we would compile-time assert that NDFILE is at least 3, but that
> > seems weirdly circumventable by sufficient accident.
> > 
> > MPASS(fdp->fd_nfiles > 3) does not guarantee we wont run into trouble,
> > has a potential to help.
> What troubles do you mean ?  Also, why > 3, and not >= 3 ?

Oops, yeah, that should have been >= 3.

I mean this kind of assertion is standard "will catch offenders, if they
happen to execute it" as opposed to making sure such offenders are not
possible to exist in the first place.

I committed this MAPSS, if I come up with something better I'll update
the code.

> Old code used fd_lastfile, which, for purpose of the assert, is also
> fine.

fd_lastfile denotes the biggest fd currently in use, so it cannot be used
in an assertion.

-- 
Mateusz Guzik <mjguzik gmail.com>