On 2013-07-08, at 2:48 PM, Reed Black <r...@unsafeword.org> wrote: > On Mon, Jul 8, 2013 at 11:00 AM, David Goulet <dgou...@ev0ke.net> wrote: >> >> Furthermore, looking at those lines of code, there is simply NO comments at >> all, >> nothing to help peer review, to explain why this or that is done that way and >> nothing linked to any design document. This is in my opinion VERY important >> that >> developers design their system/subsystem beforehand *especially* a crypto >> design >> in a public document. And, if it has to change, the design should be peer >> reviewed before even making one line of code. >> >> So, the technical critical issue, CryptoCat responded well, quickly but the >> point here is that there is a problem in terms of how development is done and >> how *little* the maintainability of the code is. > > I think there is a bigger problem in the commit messages. Looking at > the history, many are "tweak" "guehh" "update" "FIX THE BUG" and some > of those are tied to large many-purpose Swiss Army Knife commits. > > Without descriptive commit messages and single-purpose commits, > community review is highly unlikely. It takes an order of magnitude > more effort for a reviewer to suss out the intent of a code change. > The reviewer is also much less likely to ask about suspicious side > effects if he's starting with infinite possibility of intent on first > encountering the code. Few volunteers will make a routine effort. > > > Remember when someone tried slipping this into the Linux kernel? > > + if ((options == (__WCLONE|__WALL)) && (current->uid = 0)) > + retval = -EINVAL; > > Ask if something that subtle have been spotted so quickly if it were > one of many Swiss Army Knife "guehh" commits.
I'm sure "guehh" and so on are either exceptions or relate to very irrelevant commits. If they're not, then we definitely have a commit documentation problem! NK > > > I think any project that relies on community review for security needs > to first stop and ask what would make community review likely. At the > least, that's some venue for review discussion where the developers > are reading, single-function commits, and descriptive commit messages. > Does anyone know if there's something like a "best practices" page to > point to for maintaining a healthy reviewer community? > -- > Too many emails? Unsubscribe, change to digest, or change password by > emailing moderator at compa...@stanford.edu or changing your settings at > https://mailman.stanford.edu/mailman/listinfo/liberationtech -- Too many emails? Unsubscribe, change to digest, or change password by emailing moderator at compa...@stanford.edu or changing your settings at https://mailman.stanford.edu/mailman/listinfo/liberationtech
