Date: Tue, 23 Dec 2008 09:02:43 -0800 From: "David O'Brien" <obrien@FreeBSD.org> To: "M. Warner Losh" <imp@bsdimp.com> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r186291 - head/sbin/mount Message-ID: <20081223170243.GA24817@dragon.NUXI.org> In-Reply-To: <20081218.131330.63052780.imp@bsdimp.com> References: <200812181844.mBIIikVF049455@svn.freebsd.org> <20081218.131330.63052780.imp@bsdimp.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, Dec 18, 2008 at 01:13:30PM -0700, M. Warner Losh wrote: > In message: <200812181844.mBIIikVF049455@svn.freebsd.org> > : Log: > : Be a little bit more pestimistic in argument handling - check if .. > : + if (MAX_ARGS <= argc ) > : + errx(1, "Cannot process more than %d mount arguments", > : + MAX_ARGS); > : + > > This is useless. Once you've overflowed the buffer, your stack is > potentially shot, and all kinds of fun can happen downstream from > there. In particular, there's no guarantee that argc isn't corrupted > as well... s/useless/almost useless/. I fully admit its a very thin safety belt (thus the "little bit" in the commit log). The test does catches small overflows, but yes argc could also be easily damaged. I missed pulling out the change to make 'argc' static (thus avoiding the that corruption issue), I also have another patch for you (I'll send privately). > Also, the style of the check is inconsistent with other parts of the > system: > > : + if (argc > MAX_ARGS) > > would be more consistent, and not suffer from the space before the > paren problem as well :) The space before the paren was a typo :-( Actually the only other test against 'argc' is "for (i = 1; i < argc; i++)" so there is consistancy. ;-) -- -- David (obrien@FreeBSD.org)
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20081223170243.GA24817>