Date: Thu, 24 Jan 2013 10:14:14 -0500 From: John Baldwin <jhb@freebsd.org> To: Eitan Adler <eadler@freebsd.org> Cc: svn-doc-head@freebsd.org, svn-doc-all@freebsd.org, doc-committers@freebsd.org Subject: Re: svn commit: r40167 - head/en_US.ISO8859-1/books/arch-handbook/driverbasics Message-ID: <201301241014.14854.jhb@freebsd.org> In-Reply-To: <201211271530.qARFUekl097033@svn.freebsd.org> References: <201211271530.qARFUekl097033@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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 > > Log: > Modernize the example echo module and fix some style(9) issues in the > code. > > Reviewed by: alfred (code) > Reviewed by: gonzo (code) > Reviewed by: ehaupt (code) > Approved by: bcr (mentor) > > Modified: > head/en_US.ISO8859-1/books/arch-handbook/driverbasics/chapter.xml A few notes: In echo_read(): 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. In echo_write(): 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 */ should be: /* 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 together: /* * For new messages, reset the length. If appending, start * at the existing null terminator. */ if (uio->uio_offset == 0) echomsg->len = 0; else echomsg->len--; 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. -- John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201301241014.14854.jhb>