Re: [PATCH] wrapper: add workaround for open() returning EINTR

From: Jeff King <peff@peff.net>
Date: Feb 26, 2021, 06:17 UTC
Message-ID: <YDiSfXczltBTM+/T@coredump.intra.peff.net>
In-Reply-To: <YDaZtqRvdZWkyQVp@coredump.intra.peff.net>

On Wed, Feb 24, 2021 at 01:23:51PM -0500, Jeff King wrote:

Show 12 quoted lines

> > I'm not so much opposed to "enable it all the time" in general, but when
> > we already have an override of open(), like for the Windows case in
> > compat/mingw.h, I find it a bit rough to put another wrapper around it,
> > even more so since we won't have the EINTR problem on Windows due to the
> > absence of signals.
> 
> That's fair. I don't think my new wrapper would interact well with
> mingw_open(). They are both trying to #define open. I think since mine
> comes later in git-compat-util.h, it is probably overriding mingw_open()
> completely on Windows, which is quite bad (we call into my function in
> wrapper.c, but then its "#undef open" means we get the original open(),
> not the previously defined wrapper).

Thanks again for pointing out mingw_open(). The v2 I posted should avoid any bad interactions there, even if we do end up enabling it all the time.

By the way, I did notice something funny when looking at mingw_open(). It unconditionally does:

          va_start(args, oflags);
          mode = va_arg(args, int);
          va_end(args);

no matter what we see in oflags. But callers who are not passing O_CREAT will not provide a third argument at all. So probably it is loading random trash from the stack and passing it around in the "mode" variable. This doesn't hurt anything, because ultimately we pass the trash along to the real open function, which will ignore it without O_CREAT. But it is definitely undefined behavior to call va_arg() at all in this instance.

-Peff