On Fri, Oct 02, 2015 at 04:39:11PM +0300, Dan Carpenter wrote: > On Fri, Oct 02, 2015 at 06:47:35PM +0530, Chandra S Gorentla wrote: > > During the clean-up of the function, it is need to check if > > errors occurred, not the memory pointer. > > > > The bug here is that we have a use after free on the success path. It > should have been mentioned in the changelog. > > Anyway, this patch is buggy. If result == -EFAULT then it will crash. > Also this patch is really ugly. There is someone who is going to send a > correct fix (just add a return 0). > > This driver usese "do everything" style error handling. It is a bug > prone anti-pattern because doing everything is more complicated than > doing one thing. You can easily see it is bug prone, because it made > you introduce a bug, right? > > Instead the error handling should look like this: > > return 0; > > err_free_msg: > kfree(pstrMessage); > > return ret; > > There are no error paths where we need to free "pstrMessage->pvBuffer" > but if we were to add one it would look like this: > > return 0; > > err_pvbuffer: > kfree(pstrMessage->pvBuffer); > err_msg: > kfree(pstrMessage); > > return ret; > > This is a minimal, uncomplicated, no indenting, no if statement way of > unwinding. > > regards, > dan carpenter > OK. There is a problem in this patch. I will correct it, reorganize the patch series. Thank you, chandra -- 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
