Messages in this thread | | | Date | Tue, 9 Apr 2024 17:40:49 -0700 | From | "Darrick J. Wong" <> | Subject | Re: [PATCH] xfs: xattr: replace strncpy and check for truncation |
| |
On Tue, Apr 09, 2024 at 05:27:34PM -0700, Justin Stitt wrote: > On Tue, Apr 9, 2024 at 5:23 PM Justin Stitt <justinstitt@google.com> wrote: > > > > Hi, > > > > On Tue, Apr 9, 2024 at 6:32 AM Christoph Hellwig <hch@infradead.org> wrote: > > > > > > On Fri, Apr 05, 2024 at 07:45:08PM +0000, Justin Stitt wrote: > > > > - memcpy(offset, prefix, prefix_len); > > > > - offset += prefix_len; > > > > - strncpy(offset, (char *)name, namelen); /* real name */ > > > > - offset += namelen; > > > > - *offset = '\0'; > > > > + > > > > + combined_len = prefix_len + namelen; > > > > + > > > > + /* plus one byte for \0 */ > > > > + actual_len = scnprintf(offset, combined_len + 1, "%s%s", prefix, name); > > > > + > > > > + if (actual_len < combined_len) > > > > > > Shouldn't this be a != ? > > > > I guess it could be. It's a truncation check so I figured just > > checking if the amount of bytes actually copied was less than the > > total would suffice. > > > > > > > > That being said I think this is actually wrong - the attr names are > > > not NULL-terminated on disk, which is why we have the explicit > > > zero terminataion above. > > > > Gotcha, in which case we could use the "%.*s" format specifier which > > allows for a length argument. Does something like this look better? > > > > diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c > > index 364104e1b38a..1b7e886e0f29 100644 > > --- a/fs/xfs/xfs_xattr.c > > +++ b/fs/xfs/xfs_xattr.c > > @@ -206,6 +206,7 @@ __xfs_xattr_put_listent( > > { > > char *offset; > > int arraytop; > > + size_t combined_len, actual_len; > > > > if (context->count < 0 || context->seen_enough) > > return; > > @@ -220,11 +221,16 @@ __xfs_xattr_put_listent( > > return; > > } > > offset = context->buffer + context->count; > > - memcpy(offset, prefix, prefix_len); > > - offset += prefix_len; > > - strncpy(offset, (char *)name, namelen); /* real name */ > > - offset += namelen; > > - *offset = '\0'; > > + > > + combined_len = prefix_len + namelen; > > + > > + /* plus one byte for \0 */ > > + actual_len = scnprintf(offset, combined_len + 1, "%.*s%.*s", > > + prefix_len, prefix, namelen, name); > > + > > + if (actual_len < combined_len) > > + xfs_warn(context->dp->i_mount, > > + "cannot completely copy context buffer resulting in truncation"); > > > > compute_size: > > context->count += prefix_len + namelen + 1; > > --- > > I copy pasted from vim -> gmail and it completely ate all my tabs. > When I actually send the new patch, if needed, it will be formatted > correctly :)
Yeah, the "%.*s" version looks better.
> > > > > > > > > > > > How was this tested? > > > > With https://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git/about/ > > > > but using scripts + image from: https://github.com/tytso/xfstests-bld > > > > here's the output log: https://pastebin.com/V2gFhbNZ wherein I ran the > > 5 default ones (I think?): > > > > | Ran: generic/475 generic/476 generic/521 generic/522 generic/642 > > | Passed all 5 tests
Would you mind adding "-g attr,label" into the mix so that you're running all the functional tests for xattr and fs label functionality?
--D
> > > > Thanks > > Justin >
| |