Messages in this thread |  | | Date | Fri, 27 Dec 2019 00:06:34 +0000 | From | Al Viro <> | Subject | Re: [PATCH] afs: Fix compile warning in afs_dynroot_lookup() |
| |
On Thu, Dec 19, 2019 at 09:14:51PM +0800, Tiezhu Yang wrote: > Fix the following compile warning: > > CC fs/afs/dynroot.o > fs/afs/dynroot.c: In function ‘afs_dynroot_lookup’: > fs/afs/dynroot.c:117:6: warning: ‘len’ may be used uninitialized in this function [-Wmaybe-uninitialized] > ret = lookup_one_len(name, dentry->d_parent, len); > ^ > fs/afs/dynroot.c:91:6: note: ‘len’ was declared here > int len; > ^ > > Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn> > --- > fs/afs/dynroot.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/afs/dynroot.c b/fs/afs/dynroot.c > index 7503899..303f712 100644 > --- a/fs/afs/dynroot.c > +++ b/fs/afs/dynroot.c > @@ -88,7 +88,7 @@ static struct dentry *afs_lookup_atcell(struct dentry *dentry) > struct dentry *ret; > unsigned int seq = 0; > char *name; > - int len; > + int len = 0; > > if (!net->ws_cell) > return ERR_PTR(-ENOENT);
NAK. This is really, really wrong - passing zero to lookup_one_len() is always a bug. It's not any better than undefined value; if we *can* get to lookup_one_len() call without other assignments to len, we are fucked.
As it were, it's a false positive - we have if (cell) { len = cell->name_len; memcpy(name, cell->name, len + 1); } upstream of if (!cell) goto out_n;
ret = lookup_one_len(name, dentry->d_parent, len); so we can't reach the call of lookup_one_len() without having hit the assignment to len.
BTW, what guarantees that cell->name won't be "@cell"? The things would get rather interesting in that case... The same for net->sysnames in afs_lookup_atsys() - what makes sure we won't see "@sys" among those? David?
While we are at it, d = d_splice_alias(inode, dentry); if (!IS_ERR_OR_NULL(d)) { d->d_fsdata = dentry->d_fsdata; trace_afs_lookup(dvnode, &d->d_name, inode ? AFS_FS_I(inode) : NULL); } else { trace_afs_lookup(dvnode, &dentry->d_name, IS_ERR_OR_NULL(inode) ? NULL : AFS_FS_I(inode)); } is _very_ suspicious-looking - d_splice_alias() consumes an inode reference, and if it ends up failing on non-ERR_PTR() inode, the inode will be dropped by the time it returns. IOW, that AFS_FS_I(inode) in the second branch can bloody well point to already freed memory. Tracepoints: Just Say No...
|  |