Date: Tue, 28 Jan 2014 17:36:44 -0500 (EST) From: Rick Macklem <rmacklem@uoguelph.ca> To: Roman Divacky <rdivacky@freebsd.org> Cc: Dimitry Andric <dim@FreeBSD.org>, fs@FreeBSD.org Subject: Re: BUG: possible NULL pointer dereference in nfs server Message-ID: <2013799744.17924450.1390948604241.JavaMail.root@uoguelph.ca> In-Reply-To: <20140128094045.GB16311@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Roman Divacky wrote:
> > > Yea, so long as it includes a comment that states this is done to
> > > work around a stupid compiler bug.
> >
> > Ugh.
> >
> > The above has the following bugs:
> > - gross style bugs (lines longer than 80 characters)
> > - large code to do nothing
> > - would be even larger with a comment
> > - cannot actually work around any compiler bug involving abort(),
> > since it
> > has no effect on production kernels where KASSERT() is null.
> >
> > >> It is present even in your setup :) Just "objdump -d kernel |
> > >> grep
> > >> ud2" on kernel compiled
> > >> by clang.
> > >>
> > > I actually use gcc, but I believe you. I'll admit I still don't
> > > understand
> > > why having a trap instruction that never gets executed is a bad
> > > thing?
> >
> > It isn't but trying to link to the noexistent function abort() on
> > arches that don't have any trap instruction is a bad thing.
> > According
> > the above, this occurs on sparc64. It must be a gcc bug, since
> > sparc64
> > doesn't have clang. However, I couldn't get gcc on sparc64 to
> > generate
> > an abort() for *NULL. Similarly on x86 (gcc is hard to test on
> > x86,
> > since it is broken (not installed) on FreeBSD cluster machines for
>
> I was testing clang compiled kernel on sparc64.
>
> The problem is that there's nothing making sure that the NULL pointer
> dereference doesnt happen. So if someone happens to call the function
> with
> wrong flags it will crash.
>
> Thats why I want to add the KASSERT, to catch possible future cases
> when this happens.
>
> Unfortunately our KASSERT is not an assert so to remove the actual
> abort/trap/ud2 I have to remove the flag.
>
>
> Ok?
>
If it makes clang for sparc64 happy, I suppose so.
Another possible coding, which is maybe a little less ugly and might
fix the problem is:
change if (new_stp->ls_flags & NFSLCK_OPEN) {
to
if ((new_stp->ls_flags & NFSLCK_OPEN) != 0 && new_lfpp != NULL) {
rick
>
> Roman
>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?2013799744.17924450.1390948604241.JavaMail.root>