On Fri, Jan 17, 2025 at 10:23:06PM +0800, shejialuo wrote:
> On Thu, Jan 16, 2025 at 02:57:37PM +0100, Patrick Steinhardt wrote:
> > On Sun, Jan 05, 2025 at 09:49:37PM +0800, shejialuo wrote:
> > > @@ -1779,7 +1867,24 @@ static int packed_fsck(struct ref_store *ref_store,
> > > goto cleanup;
> > > }
> > >
> > > + if (strbuf_read_file(&packed_ref_content, refs->path, 0) < 0) {
> > > + /*
> > > + * Although we have checked that the file exists, there is a possibility
> > > + * that it has been removed between the lstat() and the read attempt by
> > > + * another process. In that case, we should not report an error.
> > > + */
> > > + if (errno == ENOENT)
> > > + goto cleanup;
> >
> > Unlikely, but good to guard us against that condition regardless. It's
> > still not entirely race-free though because the file could meanwhile
> > have changed into a symlink, and we wouldn't notice now. We could fix
> > that by using open(O_NOFOLLOW), fstat the returne file descriptor and
> > then use `strbuf_read()` to slurp in the file.
> >
>
> Would this be too complicated for us to avoid race condition and we will
> introduce a lot of code to handle above logic. Because there is a
> possibility that when finishing reading the file content to the memory,
> the file could be changed into a symlink and we cannot notice. So, I
> wanna say we can't avoid race condition totally. It would be good if we
> avoid race, but what I am concern about here is that we would make the
> logic too complicated. So, could we make it unchanged?
It would ultimately only be two additional function calls, so I don't
think it's going to add a ton of complexity. Whether things are changing
_after_ we have opened and read the file is a different issue, and I
agree that we shouldn't have to care about that case. What we're after
is whether things are correct when running consistency checks, it's
always a possibility that e.g. the packed-refs file gets rewritten while
we do it.
Patrick
