On Mon, Jan 26, 2015 at 08:52:09PM -0800, Omar Sandoval wrote: > Hi, Dan, > > On Sat, Jan 24, 2015 at 10:31:24PM +0300, Dan Carpenter wrote: > > If posix_acl_create() returns an error code then "*acl" and > > "*default_acl" can be uninitialized or point to freed memory. This > > causes problems in some of the callers where it is expected that they > > are NULL on error. For example, ocfs2_reflink() has a bug. > > > > fs/ocfs2/refcounttree.c:4329 ocfs2_reflink() > > error: potentially using uninitialized 'default_acl'. > > > > I have re-written this function and re-arranged things so that they are > > set to NULL at the start and then only set to a valid pointer at the end > > of the function. > > > I'm inclined to blame ocfs2 and not posix_acl_create() here. I'd imagine > that most C programmers' intuition is generally not to trust any return > parameters when the return value is an error. Accordingly, a quick scan > of the tree showed that all of the other users of posix_acl_create() are > doing it correctly and only calling posix_acl_release() when it returns > success. Both ocfs2_reflink() and posix_acl_create() are ugly. ocfs2_reflink() uses an "out:" label. Whenever you see an "out:" label then, hopefully, people are just too lazy to name their labels but a lot of the time an out label means the code is going to do something stupid. In this case, it's doing "one err" style error handling where it just has one error label that tries to free everything including the stuff that was never allocated. One Err Bugs like this are very common in the kernel. Still it's bad to return freed pointers because you know that it's going to cause double frees. And also posix_acl_create() was messy in general. Both functions should be cleaned up but I am too busy to clean up the whole kernel. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
