Re: svn commit: r40167 - head/en_US.ISO8859-1/books/arch-handbook/driverbasics
On Tuesday, November 27, 2012 10:30:40 am Eitan Adler wrote:
> Author: eadler
> Date: Tue Nov 27 15:30:40 2012
> New Revision: 40167
> URL: http://svnweb.freebsd.org/changeset/doc/40167
> Modernize the example echo module and fix some style(9) issues in the
> Reviewed by: alfred (code)
> Reviewed by: gonzo (code)
> Reviewed by: ehaupt (code)
> Approved by: bcr (mentor)
A few notes:
amt = MIN(uio->uio_resid, echomsg->len - uio->uio_offset);
You should probably make 'amt' a ssize_t since uio_resid is now a ssize_t.
Also, I'm not sure it is safe to remove the check for uio_offset being >
echmosg->len. On 32-bit platforms, uio_offset is an off_t (64-bit), but 'len'
is an int and you are assigning it to an int (or ssize_t). I think you can
get a uio_offset such that you overflow and when you truncate to 32-bits you
end up with a positive value for 'echomsg->len - uio->uio_offset'. This
coupled with a uio_resid larger than 'echomsg->len' (easy to do) can result in
a buffer overrun. I would probably write this as:
amt = MIN(uio->uio_resid, uio->uio_offset >= echomsg->len ? 0 :
echomsg->len - uio->uio_offset);
Rather than the previous version that tested the result of the subtraction.
You should also not adjust uio_offset manually. uiomove() already does that.
The duplicate 'Copy the string in ...' comment at the top of this function
should be removed.
It's not clear to me why you forbade random access? Though it is fine if you
want to do that.
style(9) says single-line comments shouldn't span multiple lines, hence:
* This is new message, reset length
/* This is a new message, reset length. */
IIRC, It is a null character, not a NULL character (NULL is only for
pointers). Also, I think that should be in an else, so putting all that
* For new messages, reset the length. If appending, start
* at the existing null terminator.
if (uio->uio_offset == 0)
echomsg->len = 0;
Again, do not adjust uio_offset manually, you just did it twice. In fact,
this will do the wrong thing if you get an EFAULT error. The right thing to
do to handle partial copies, etc. is this:
error = uiomove(...);
/* Now we need to null terminate, then record the length. */
echomsg->len = uio->uio_offset + 1;
echomsg->msg[uio->uio_offset] = '\0';
Other pre-existing nits:
This driver shouldn't reference 'kmalloc' in a comment, and that entire
comment can probably just be removed.
The DEV_MODULE() macro should have spaces after the comma operator.
Received on Thu Jan 24 2013 - 19:52:31 UTC