Re: [PATCH v2 3/8] tools: Add directory checker for updates

Demi Marie Obenour
2025-11-13T18:03:57Z

[-- Attachment #1.1.1: Type: text/plain, Size: 3239 bytes --]

On 11/13/25 13:01, Alyssa Ross wrote:
> Demi Marie Obenour <demiobenour@gmail.com> writes:
>
>> On 11/13/25 08:21, Alyssa Ross wrote:
>>> Demi Marie Obenour <demiobenour@gmail.com> writes:
>>>
>>>> +static void checkdir(int fd)
>>>> +{
>>>> + DIR *d = fdopendir(fd);
>>>> + if (d == NULL)
>>>> + err(EXIT_FAILURE, "fdopendir");
>>>> + // If there is an I/O error while there are dirty pages outstanding,
>>>> + // the dirty pages are silently discarded. This means that the contents
>>>> + // of the filesystem can change behind userspace's back. Flush all
>>>> + // dirty pages in the filesystem with the directory to prevent this.
>>>> + if (syncfs(fd) != 0)
>>>> + err(EXIT_FAILURE, "syncfs");
>>>> + for (;;) {
>>>> + errno = 0;
>>>> + struct dirent *entry = readdir(d);
>>>> + if (entry == NULL) {
>>>> + if (errno)
>>>> + err(EXIT_FAILURE, "readdir");
>>>> + break;
>>>> + }
>>>> + assert(entry->d_reclen > offsetof(struct dirent, d_name));
>>>
>>> Would be a POSIX violation for d_name not to be valid I think.
>>
>> Indeed it would be, but I preferred to check that explicitly.
>>
>>>> + size_t len = strnlen(entry->d_name, entry->d_reclen - offsetof(struct dirent, d_name));
>>>
>>> POSIX also guarantees a terminating null byte, so strlen() would be fine here.
>>
>> I preferred to double-check libc, but if you prefer to get rid of
>> those assertions I'm okay with that.
>
> I don't think it's Spectrum's place to confirm libc is POSIX compliant.
> Adding these checks isn't free, because it's more stuff to get past to
> understand what's going on.

Oh, that makes sense.

>>>> + if (entry->d_name[0] == '.')
>>>> + if (len == 1 || (len == 2 && entry->d_name[1] == '.'))
>>>> + continue;
>>>> + unsigned char c = (unsigned char)entry->d_name[0];
>>>> + if (!((c >= 'A' && c <= 'Z') ||
>>>> + (c >= 'a' && c <= 'z')))
>>>> + errx(EXIT_FAILURE, "Filename must begin with an ASCII letter");
>>>
>>> Would the comparison not be valid without the cast?
>>
>> It would be in this case, but the subsequent cast to int in the error
>> path assumes that the cast was done. Signed characters are much
>> trickier and casting to unsigned makes the code easier to reason about.
>
> Is it safe to assume 'A' etc. are representable and comparable as
> unsigned values? (I'm sure it is in practice, but I'd like it if I
> could be confident this is being done strictly correctly.)

I don't know if the C standard requires it, but we assume it.

>>>> + if (entry->d_type != DT_REG)
>>>> + errx(EXIT_FAILURE, "Entry contains non-regular file %s", entry->d_name);
>>>> + }
>>>> + closedir(d);
>>>> +}
>>>> +
>>>> +int main(int argc, char **argv)
>>>> +{
>>>> + for (int i = 1; i < argc; ++i) {
>>>
>>>> + int fd = open(argv[i], O_DIRECTORY|O_RDONLY|O_CLOEXEC|O_NOFOLLOW);
>>>
>>> Wasn't O_NOFOLLOW going to be removed here?
>>
>> It should never be called on a symlink, so if it does that is a bug
>> in the caller. I can remove it if you prefer, though.
>
> I don't think it's the place of this program to put constraints on its
> caller like that.

Makes sense.
--
Sincerely,
Demi Marie Obenour (she/her/hers)

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 7253 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]