Junio C Hamano <gitster@xxxxxxxxx> wrote: > Junio C Hamano <gitster@xxxxxxxxx> writes: > > > Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> writes: > > > >> On Thu, Oct 27, 2016 at 4:36 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote: > >>> > >>> Would the best endgame shape for this function be to open with > >>> O_NOATIME (and retry without), and then add CLOEXEC with fcntl(2) > >>> but ignoring an error from it, I guess? That would be the closest > >>> to what we historically had, I would think. > >> > >> I think that's the best model. Actually, I would flip the order of flags. O_CLOEXEC is more important from a correctness standpoint. > > OK, so perhaps like this. > > Hmph. This may not fly well in practice, though. > > To Unix folks, CLOEXEC is not a huge correctness issue. A child > process may hold onto an open file descriptor a bit longer than the > lifetime of the parent but as long as the child eventually exits, I'm not too familiar with C internals of git; but I know we use threads in some places, and fork+execve in others. If our usage of threads and execve intersects, and we run untrusted code in an execve-ed child, then only having cloexec on open() will save us time when auditing for leaking FDs. fcntl(fd, F_SETFD, O_CLOEXEC) is racy in if there are other threads doing execve; so I wouldn't rely on it as a first choice. So I suppose something like this: static int noatime = 1; int fd = open(... | O_CLOEXEC); ...error checking and retrying... if (fd >= 0 && noatime && fcntl(fd, F_SETFL, O_NOATIME) != 0) noatime = 0; return fd;