On Mon, Mar 23, 2015 at 03:24:09PM +0200, Jouni Malinen wrote: > > This function should be a list of commands in a row with tiny detours > > for exceptions and error handling. It messes everyone up if the success > > path is hidden somewhere in the middle and it leads to bugs like this. > > I'm not sure that would be behind this specific bug, It is though. 1) The code here: err = crypto_aead_setkey(tfm, key, key_len); if (!err) return tfm; That looks like normal error handling code so your mind glosses over it. Reviewers are human and this is a blind spot. 2) If you do it in normal kernel style then the return would have been at level 1 indent so the static checkers would have complained about the unreachable code. This was caught with a static checker anyway, I suppose, but that static check has a lot more false positives so I'm not sure I can release it. 3) This code is just confusing. The error and the success paths are twisted together. After the first allocation then the exclusive parts of the success path move from indent level 1 to indent level 2. Confusing code is more likely to have bugs. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
