Re: [PATCH v2 2/5] reftable/stack: refactor reloading to use file descriptor

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 3799 bytes --]

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]