On Sun, Jan 14, 2024 at 05:14:24AM -0500, Jeff King wrote:
> On Thu, Jan 11, 2024 at 11:06:43AM +0100, Patrick Steinhardt wrote:
>
> > We're about to introduce a stat(3P)-based caching mechanism to reload
> > the list of stacks only when it has changed. In order to avoid race
> > conditions this requires us to have a file descriptor available that we
> > can use to call fstat(3P) on.
> >
> > Prepare for this by converting the code to use `fd_read_lines()` so that
> > we have the file descriptor readily available.
>
> Coverity noted a case with this series where we might feed a negative
> value to fstat(). I'm not sure if it's a bug or not.
>
> The issue is that here:
>
> > @@ -329,9 +330,19 @@ static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,
> > if (tries > 3 && tv_cmp(&now, &deadline) >= 0)
> > goto out;
> >
> > - err = read_lines(st->list_file, &names);
> > - if (err < 0)
> > - goto out;
> > + fd = open(st->list_file, O_RDONLY);
> > + if (fd < 0) {
> > + if (errno != ENOENT) {
> > + err = REFTABLE_IO_ERROR;
> > + goto out;
> > + }
> > +
> > + names = reftable_calloc(sizeof(char *));
> > + } else {
> > + err = fd_read_lines(fd, &names);
> > + if (err < 0)
> > + goto out;
> > + }
>
> ...we might end up with fd as "-1" after calling open() on the list
> file. For most errors we'll jump to "out", which makes sense. But if we
> get ENOENT, we keep going with an empty file-list, which makes sense.
>
> But we then do other stuff with "fd". I think this case is OK:
>
> > @@ -356,12 +367,16 @@ static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,
> > names = NULL;
> > free_names(names_after);
> > names_after = NULL;
> > + close(fd);
> > + fd = -1;
>
> We only get here if reftable_stack_reload_once() returned an error,
> which it won't do since we feed it a blank set of names (and anyway,
> close(-1) is a harmless noop).
>
> But if we actually get to the end of the function, it's more
> questionable. As of this patch, it's OK:
>
> > delay = delay + (delay * rand()) / RAND_MAX + 1;
> > sleep_millisec(delay);
> > }
> >
> > out:
> > + if (fd >= 0)
> > + close(fd);
> > free_names(names);
> > free_names(names_after);
> > return err;
>
> But in the next patch we have this hunk:
>
> > @@ -374,7 +375,11 @@ static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,
> > sleep_millisec(delay);
> > }
> >
> > + stat_validity_update(&st->list_validity, fd);
> > +
> > out:
> > + if (err)
> > + stat_validity_clear(&st->list_validity);
> > if (fd >= 0)
> > close(fd);
> > free_names(names);
>
> which means we'll feed a negative value to stat_validity_update(). I
> think this may be OK, because I'd imagine the only sensible thing to do
> is call stat_validity_clear() instead. And using a negative fd means
> fstat() will fail, which will cause stat_validity_update() to clear the
> validity struct anyway. But I thought it was worth double-checking.
Good catch, and thanks a lot for double-checking. I was briefly
wondering whether this behaviour is actually specified by POSIX. In any
case, fstat(3P) explicitly documents `EBADF` as:
The fildes argument is not a valid file descriptor.
That makes me think that this code is indeed POSIX-compliant, as
implementations are expected to handle invalid file descriptors via this
error code.
So overall this works as intended, even though I would not consider it
to be the cleanest way to handle this. Unless you or others think that
this should be refactored I'll leave it as-is for now though.
Patrick
Attachment:
signature.asc
Description: PGP signature
