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.
-Peff
