On Sun, Aug 11, 2019 at 11:01:36AM +1000, Dave Chinner wrote:
> On Sat, Aug 10, 2019 at 11:45:35PM +0200, Holger Hoffstätte wrote:
> >
> > Hi Dave -
> >
> > great patch but I found something that seems off in xfs_attr3_leaf_getvalue:
>
> It's not a "great patch" if there's something wrong with it. :/
>
> > > @@ -2378,31 +2403,23 @@ xfs_attr3_leaf_getvalue((..snip..)
> > > + if (args->flags & ATTR_KERNOVAL) {
> > > args->valuelen = args->rmtvaluelen;
> > > + return 0;
> > > }
> > > - return 0;
> > > + return xfs_attr_copy_value(args, NULL, args->rmtvaluelen);
> >
> > With gcc9 I get:
> >
> > CC fs/xfs/libxfs/xfs_attr_leaf.o
> > In function 'xfs_attr_copy_value',
> > inlined from 'xfs_attr3_leaf_getvalue' at fs/xfs/libxfs/xfs_attr_leaf.c:2425:9:
> > fs/xfs/libxfs/xfs_attr_leaf.c:421:2: warning: argument 2 null where non-null expected [-Wnonnull]
> > 421 | memcpy(args->value, value, valuelen);
> > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > In file included from ./arch/x86/include/asm/string.h:5,
> > from ./include/linux/string.h:20,
> > from ./include/linux/uuid.h:12,
> > from ./fs/xfs/xfs_linux.h:10,
> > from ./fs/xfs/xfs.h:22,
> > from fs/xfs/libxfs/xfs_attr_leaf.c:7:
> > fs/xfs/libxfs/xfs_attr_leaf.c: In function 'xfs_attr3_leaf_getvalue':
> > ./arch/x86/include/asm/string_64.h:14:14: note: in a call to function 'memcpy' declared here
> > 14 | extern void *memcpy(void *to, const void *from, size_t len);
> > | ^~~~~~
> >
> > and sure enough, the NULL "value" arg is and passed as-is to memcpy in
> > xfs_attr_copy_value.
>
> "sure enough", eh?
>
> > Maybe you meant to sanitize the value when it's NULL?
>
> Nope - look at the code:
>
> args->rmtvaluelen = be32_to_cpu(name_rmt->valuelen);
> >>>> args->rmtblkno = be32_to_cpu(name_rmt->valueblk);
> args->rmtblkcnt = xfs_attr3_rmt_blocks(args->dp->i_mount,
> args->rmtvaluelen);
> if (args->flags & ATTR_KERNOVAL) {
> args->valuelen = args->rmtvaluelen;
> return 0;
> }
> return xfs_attr_copy_value(args, NULL, args->rmtvaluelen);
> }
>
> And the relevant code in xfs_attr_copy_value() does:
>
> /* remote block xattr requires IO for copy-in */
> >>>> if (args->rmtblkno)
> >>>> return xfs_attr_rmtval_get(args);
>
> memcpy(args->value, value, valuelen);
> return 0;
> }
>
> The memcpy() is never reached in this case. Hence the compiler
> warning is a false positive and the code is not going to crash here.
>
> Regardless, I'm going to have to change the code because I doubt gcc
> will ever be smart enough to understand the code flow as it stands.
> We have to do this every so often to avoid false positive
> uninitialised variable warnings, so it's not like working around
> compiler issues is something new.
>
> I'll post an updated version tomorrow....
Ping?
--D
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
