On Wed, Mar 04, 2020 at 04:20:06PM -0700, Tracey Emery wrote:
> On Wed, Mar 04, 2020 at 09:04:10PM +0100, Stefan Sperling wrote:
> > But don't worry about any of that now. This race can be dealt with later.
> > Your original worktree.c patch doesn't make anything worse, and it was a
> > better starting position for further changes in this direction, I think.
> 
> Just one more before we revert to something like the first patch. Using
> scandir, we can avoid the race as we iterate the directories. But,
> perhaps this operation is too expensive on large dirs, since there would
> be a lot of malloc and freeing for each file.


> +	parent = dirname(ondisk_path);
> +	if (parent == NULL) {
> +		err = got_error_from_errno2("dirname", ondisk_path);
> +		goto done;
> +	}

> +	while(parent && strcmp(parent, root_path) != 0) {
> +		if (dirfd != -1) {
> +			dir_cnt = scandir(parent, &sd_dent, NULL, NULL);
> +			if (dir_cnt == -1) {
> +				err = got_error_from_errno2("scandir", parent);
> +				goto done;
> +			}

But, you see, scandir opens the directory by path, rather than via the
file descriptor which was passed in from the status walker!

So we can still lose the race and delete entries from an unrelated directory
if our original directory has been replaced since the status walk began to
crawl it.

We can only win this race with openat + AT_DIRECTORY, as far as I can see.

> Personally, I think I would opt for a fixed up version of the first
> patch to avoid all the memory shuffle, then work on a better total fix.

We can take time to find a good fix for the race.

You are fixing user-visible functionality, and the race is about correctness.
Quite different problems. Note also that the status walk has not yet been
converted to openat() everywhere. There's lots more work left to do.