Date: Tue, 5 Nov 2013 18:02:30 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: John-Mark Gurney <jmg@funkthat.com> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, Eitan Adler <eadler@FreeBSD.org> Subject: Re: svn commit: r257646 - head/lib/libc/string Message-ID: <20131105174205.E6267@besplex.bde.org> In-Reply-To: <20131105053158.GQ73243@funkthat.com> References: <201311041905.rA4J5WT0097968@svn.freebsd.org> <20131105053158.GQ73243@funkthat.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, 4 Nov 2013, John-Mark Gurney wrote:
> Eitan Adler wrote this message on Mon, Nov 04, 2013 at 19:05 +0000:
>>
>> Log:
>> Use OpenBSD's revamped description of strlcpy and strlcat.
> ...
> Can we add a warning that it is not safe to just simply replace strncpy
> with strlcpy? strncpy does something useful in that it NULs out the
> remaining buffer, which when coping strings from/to kernel buffers
> prevent information leaks, so I'd argue that strlcpy can be used
> incorrectly just as strncpy can be...
Most kernel code handles this by bzeroing the whole buffer before filling
it. This is usually necessary anyway, for initializing unnamed padding
in structs. strncpy() could only do it all if the syscall is returning
a single string.
Howver, it is difficult to see that buffers are pre-zeroed before
strlcpy()ing into them. A quick grep showed the following nonsense:
% static void
% fill_kinfo_thread(struct thread *td, struct kinfo_proc *kp, int preferthread)
% {
% struct proc *p;
%
% p = td->td_proc;
% kp->ki_tdaddr = td;
% PROC_LOCK_ASSERT(p, MA_OWNED);
%
% if (preferthread)
% PROC_SLOCK(p);
% thread_lock(td);
% if (td->td_wmesg != NULL)
% strlcpy(kp->ki_wmesg, td->td_wmesg, sizeof(kp->ki_wmesg));
% else
% bzero(kp->ki_wmesg, sizeof(kp->ki_wmesg));
% strlcpy(kp->ki_tdname, td->td_name, sizeof(kp->ki_tdname));
I think the whole buffer (*kp) is pre-zeroed, so the strlcpy()'s are
correct. But in that case the bzero() is nonsense (unless ki_wmesg
is aliased and might have been scribbled on with compatibility cruft
if and only if this can only happen when the bzero() case is reached).
This function also a strlcpy()/bzero() pair for ki_lockname, and an
apparently-unnecessary flags clearing for the bzero() case of that.
Several other variables are only touched conditionally. The ki_tdname
variable visible in the above used to be one of these.
This function also has a laborious explicit initialization to 0. There
is just 1 of these (for ki_rqindex), and this one is especially silly
since it is attached to a verbose comment which says that it is unusable
now and was never used by ps etc.
I like to intentionally omit all explicit initializations to 0 after
pre-zeroing.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20131105174205.E6267>