| Message ID | 36b9f6b6240686cc5b0a761b889614fc31f01d34.1704966670.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c5b5d5fbbc43364a3d3c0aedf9e984a0ffe04537 |
| Headers | show |
| Series | reftable: optimize I/O patterns | expand |
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
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
On Mon, Jan 15, 2024 at 11:03:37AM +0100, Patrick Steinhardt wrote: > > 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. Thanks for confirming. I think we can leave your patch as-is. If anything, I would say that stat_validity_update() should check for "fd < 0" itself. Not because I think fstat() is unlikely to behave differently on some platform, but simply because it more clearly documents the expectation. -Peff
Jeff King <peff@peff.net> writes: >> 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. > > Thanks for confirming. I think we can leave your patch as-is. If > anything, I would say that stat_validity_update() should check for "fd < > 0" itself. Not because I think fstat() is unlikely to behave differently > on some platform, but simply because it more clearly documents the > expectation. Thanks, I agree with your point that we should avoid calling system functions that take a file descriptor with a known-invalid (like negative) one.
diff --git a/reftable/stack.c b/reftable/stack.c index bf869a6772..b1ee247601 100644 --- a/reftable/stack.c +++ b/reftable/stack.c @@ -308,6 +308,7 @@ static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st, struct timeval deadline; int64_t delay = 0; int tries = 0, err; + int fd = -1; err = gettimeofday(&deadline, NULL); if (err < 0) @@ -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; + } err = reftable_stack_reload_once(st, names, reuse_open); if (!err) @@ -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; 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;
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. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- reftable/stack.c | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-)