Re: [PATCH v2 2/5] reftable/stack: refactor reloading to use file descriptor
- From
Jeff King <peff@peff.net>
- Date
- Jan 14, 2024, 10:14 UTC
- Message-ID
- <20240114101424.GA1196682@coredump.intra.peff.net>
- In-Reply-To
- <36b9f6b6240686cc5b0a761b889614fc31f01d34.1704966670.git.ps@pks.im>
On Thu, Jan 11, 2024 at 11:06:43AM +0100, Patrick Steinhardt wrote:
Show 7 quoted lines
> 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:
Show 20 quoted lines
> @@ -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:
Show 6 quoted lines
> @@ -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:
Show 10 quoted lines
> 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:
Show 12 quoted lines
> @@ -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