Re: [PATCH] xfs: mark sb_fname as nonstring

[Arnd Bergmann]

On Fri, May 25, 2018 at 6:53 PM, Eric Sandeen <sandeen@xxxxxxxxxx> wrote:
> On 5/25/18 10:14 AM, Arnd Bergmann wrote:
>>
>> gcc-8 reports two warnings for the newly added getlabel/setlabel code:
>
>
> Thanks for catching these.
>
> The patch summary confuses me, what does "mark sb_fname as nonstring"
> mean in the context of the actual patch?

My mistake. I tried a few different approaches and ended up using the
subject line from an earlier version with a later patch.

The 'nonstring' annotation is a variable attribute that gets gcc-8
to shut up about -Wstringop-truncation warnings, and is intended
to mark those character arrays that are not expected to be
null-terminated but still used with strncpy().

>>         spin_unlock(&mp->m_sb_lock);
>>         /* xfs on-disk label is 12 chars, be sure we send a null to user
>> */
>>         label[XFSLABEL_MAX] = '\0';
>> -       if (copy_to_user(user_label, label, sizeof(sbp->sb_fname)))
>> +       if (copy_to_user(user_label, label, sizeof(label)))
>
>
>
> ok.  (odd how this is ok for copy_to_user but not for strncpy above) :)

No idea. Maybe the gcc bug only happens with struct members but
not local variables?

>>                 return -EFAULT;
>>         return 0;
>>   }
>> @@ -1860,7 +1860,7 @@ xfs_ioc_setlabel(
>>         spin_lock(&mp->m_sb_lock);
>>         memset(sbp->sb_fname, 0, sizeof(sbp->sb_fname));
>> -       strncpy(sbp->sb_fname, label, sizeof(sbp->sb_fname));
>> +       memcpy(sbp->sb_fname, label, len);
>
>
> Hm but len = strnlen(label, XFSLABEL_MAX + 1);
> which could be one longer than sbp->sb_fname, no?

We have an explicit check for that, so I think it's ok:

        if (len > sizeof(sbp->sb_fname))
                return -EINVAL;

       Arnd