kern/106255: [msdosfs] : correct setting of archive flag

[Bruce Evans]

On Mon, 4 Dec 2006, Rene Ladan wrote:

> Bruce Evans schreef:
>> On Sun, 3 Dec 2006, Rene Ladan wrote:
>>
>>>> Description:
>>> The MSDOS file system has an archive bit in the flags field.  This bit
>>> roughly corresponds to the archive flag on the UFS file system.
>>> However, it is set the wrong way around: the flag should be set when
>>> the bit is present, and cleared when the bit is absent.
>>
>> The comment in msdosfs/direntry.h says that ATTR_ARCHIVE means that
>> the file is new or modified (in other words, not archived), while the
>> comment in sys/stat.h says that SF_ARCHIVED means that the file is
>> archived, but I think both mean that it is archived.
>>
> Indeed, the MSDOS archive bit means that the user should archive the file.

Not indeed.  The MSDOS archive bit does mean that the file needs to be
archived, but this means that it has the opposite sense to SF_ARCHIVED
and the main bug is in the patch in the PR.

>>> --- msdosfs_vnops.c    Mon Nov  6 14:41:57 2006
>>> +++ msdosfs_vnops.c.rene    Sun Dec  3 11:58:47 2006
>>> @@ -352,7 +352,7 @@
>>>         vap->va_ctime = vap->va_mtime;
>>>     }
>>>     vap->va_flags = 0;
>>> -    if ((dep->de_Attributes & ATTR_ARCHIVE) == 0)
>>> +    if (dep->de_Attributes & ATTR_ARCHIVE)
>>>         vap->va_flags |= SF_ARCHIVED;
>>>     vap->va_gen = 0;
>>>     vap->va_blocksize = pmp->pm_bpcluster;
>>
>> This only fixes the reporting of the flag.  msdosfs still maintains
>> the flag perfectly backwards (except DETIMES() is missing setting of
>> it for for all changes -- I think all changes to metadata except
>> possibly to atimes should set it to be perfectly backwards and clear
>> it to be correct).
>>
> Thanks, I'll look into that.

The above actually only breaks the reporting of the flag.  ATTR_ARCHIVE
has the opposite sese to SF_ARCHIVED, so !ATTR_ARCHIVED must be converted
to SF_ARCHIVED as in the original code above, and the reverse conversion
must be done when setting msdosfs attributes from FreeBSD attributes
(as done in msdosfs_setattr()).  At the user level, thus means that
when ls -o displays "attr", it means that the file is archived, but
when an msdosfs's attrib utility displays "A" it means that the file
is not archived.

The main bug here seems to be just for directories.  The archive
attribute doesn't apply for directories, and the above doesn't have
a special case for directories, so the above tests garbage for the
directory case.  The garbage is usually (maybe always?) 0, so
SF_ARVCHIVED is usually set for directories and ls -o displays "attr".
This is consistent with attrib not displaying "A" but strange.

msdosfs_setattr() is more careful with the archive bit for directories,
but still wrong.  It silently ignores attempts to set this bit.  Thus
applications like cp -p succeed where they should fail, masking the
bug in msdosfs_getattr(): msdosfs_getattr() bogusly turns the absence
of an ATTR_ARCHIVE bit into a setting of SF_ARCHIVED; cp -p tries to
preserve this setting but cannot, and the error goes undetected since
msdosfs_setattr() silently ignores it.

More in another reply.

Bruce