Date: Sun, 26 Jan 2014 05:29:40 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Dimitry Andric <dim@FreeBSD.org> Cc: Roman Divacky <rdivacky@FreeBSD.org>, fs@FreeBSD.org Subject: Re: BUG: possible NULL pointer dereference in nfs server Message-ID: <20140126051258.M1750@besplex.bde.org> In-Reply-To: <97D8A7EA-12F3-4EFC-A933-BD8528B6306A@FreeBSD.org> References: <1577222508.16050114.1390610315654.JavaMail.root@uoguelph.ca> <97D8A7EA-12F3-4EFC-A933-BD8528B6306A@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, 25 Jan 2014, Dimitry Andric wrote:
> On 25 Jan 2014, at 01:38, Rick Macklem <rmacklem@uoguelph.ca> wrote:
> ...
> If it inlines this, the result looks approximately like:
>
> 1 {
> 2 fhandle_t *fhp = NULL;
> 3 struct nfslockfile *new_lfp;
> 4 int error;
> 5
> 6 if (new_stp->ls_flags & NFSLCK_OPEN) {
> 7 new_lfp = *NULL;
> 8 fhp = &new_lfp->lf_fh;
> 9 } else if (&nfh) {
> 10 fhp = &nfh;
> 11 } else {
> 12 panic("nfsrv_getlockfh");
> 13 }
> 14 error = nfsvno_getfh(vp, fhp, p);
> 15 NFSEXITCODE(error);
> 16 getlckret = error;
> 17 }
>
> The code in line 7 is the problematic part. Since this is undefined,
> the compiler inserts a trap instruction here. I think the problem Roman
> encountered is that on sparc64, there is no equivalent to x86's ud2
> instruction, so it inserts a call to abort() instead, and that function
> is not available in kernel-land.
Compiler bug. abort() is not available in freestanding implementations.
The behaviour is only undefined if the null pointer is dereferenced at
runtime, so it doesn't include failing to link to abort() at compile
time.
> ...
>> Sorry, I'm not a compiler guy, so I don't know why a compiler would
>> generate a trap instruction, but since new_lfpp is never NULL when
>> this is executed, I don't see a problem.
>>
>> If others feel that this needs to be re-coded, please let me know what
>> you think the code should look like? (A test for non-NULL with a panic()
>> before it is used?)
>>
>> Is a trap instruction that never gets executed a problem?
>
> It's better to avoid undefined behavior in any case. Just add a NULL
> check, that should be sufficient.
That might only add bloat and unimprove debugging. Since the null pointer
case cannot happen, it cannot be handed properly. It can be mishandled in
the following ways:
- return an error, so that all callers have to handle the null pointer case
that can't happen. If the compiler is too smart, it will notice more
undefined behaviour (that can't happen) in callers and "force" you to
handle it there too
- KASSERT() that the pointer cannot be null. Then:
- on production systems where KASSERT() is null, this won't work around
the compiler bug. Use a panic() instead. To maximize source code
bloat, ifdef all of this.
- when KASSERT() is not null, it will work around the compiler bug.
If the case that can't happen actually happens, then this unimproves
the debugging by messing up stack traces and turning restartable
null pointer or SIGILL traps to non-restartable panics.
Optimizations that replace a large block of code ending with a
null pointer trap by a single unimplemented instruction would
probably break restarting anyway.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140126051258.M1750>