bug#73444: 30.0.50; mingw-w64: Emacs uses CRT's `read` when _FORTIFY_SOURCE > 0

bug#73444: 30.0.50; mingw-w64: Emacs uses CRT's `read` when _FORTIFY_SOURCE > 0

[Óscar Fuentes]

On Onssee
When the macro _FORTIFY_SOURCE > 0, mingw-64 provides an inline
definition of `read` on io.h:

__mingw_bos_extern_ovr
int read(int __fh, void * __dst, unsigned int __n)
{
  return _read(__fh, __dst, __n);
}

*Before* #including io.h, ms-w32.h does

#define read    sys_read

so the inline definition of read above ends like this:

int sys_read(int __fh, void * __dst, unsigned int __n)
{
  return _read(__fh, __dst, __n);
}

As the result of this, the above definition is used instead of Emacs'
definition of sys_read. The resulting Emacs is unusable due to problems
handling subprocesses.

A hack that avoids this consists on doing something like:

#define read dummy_read
// etc
#include <io.h>
// etc
#undef read
#define read sys_read
int sys_read (int, char *, unsigned int);

or simpler but untested:

#define _read sys_read
// etc
#include <io.h>
// etc

Either way it is necessary to condition the hack on the value of
_FORTIFY_SOURCE.

More generally, the way Emacs/NT overrides the CRT functions is
susceptible to break anytime upstream does something like, this case,
adding an inline definition, or some other unanticipated change. AFAIK
the C standard says that precisely what Emacs is doing incurs on
undefined behavior.

Any ideas about how to future-proof the solution for this problem?

BTW, the initial bug report for this was in March 2023 and only today
was succesfully analyzed (1) This gives an idea of how problematic this
practice of redefining standard functions can be.

1. https://github.com/msys2/MINGW-packages/issues/17343#issuecomment-2368903501

bug#73444: 30.0.50; mingw-w64: Emacs uses CRT's `read` when _FORTIFY_SOURCE > 0

[Óscar Fuentes]

BTW, #63752 was filed about the observable problems when Emacs is built
with _FORTIFY_SOURCE on mingw-w64, which documents an ensuing effort to
try to identify the cause, unsuccessfully.

bug#73444: 30.0.50; mingw-w64: Emacs uses CRT's `read` when _FORTIFY_SOURCE > 0

[Eli Zaretskii]

> From: Óscar Fuentes <ofv@wanadoo.es>
> Date: Tue, 24 Sep 2024 00:29:37 +0200
> 
> BTW, #63752 was filed about the observable problems when Emacs is built
> with _FORTIFY_SOURCE on mingw-w64, which documents an ensuing effort to
> try to identify the cause, unsuccessfully.

I don't think I follow.  Are you saying that these two bugs should be
merged, because solving this problem with 'read' solves also the
problems reported in bug#63752?  Or are there other problems with
_FORTIFY_SOURCE that are still left unresolved if the issue with
'read' is fixed?

bug#73444: 30.0.50; mingw-w64: Emacs uses CRT's `read` when _FORTIFY_SOURCE > 0

[Óscar Fuentes]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Óscar Fuentes <ofv@wanadoo.es>
>> Date: Tue, 24 Sep 2024 00:29:37 +0200
>> 
>> BTW, #63752 was filed about the observable problems when Emacs is built
>> with _FORTIFY_SOURCE on mingw-w64, which documents an ensuing effort to
>> try to identify the cause, unsuccessfully.
>
> I don't think I follow.  Are you saying that these two bugs should be
> merged, because solving this problem with 'read' solves also the
> problems reported in bug#63752?  Or are there other problems with
> _FORTIFY_SOURCE that are still left unresolved if the issue with
> 'read' is fixed?

#63752 describes the simptons and tries to identify the cause, this bug
(#73444) provides the cause and discusses a possible fix, hopefully one
that covers the general case and not just `read'.

I was not aware of the existence of #63752 when I created this bug. As
what to do with #63752, I have no preferences.

bug#73444: 30.0.50; mingw-w64: Emacs uses CRT's `read` when _FORTIFY_SOURCE > 0

[Eli Zaretskii]

> From: Óscar Fuentes <ofv@wanadoo.es>
> Cc: 73444@debbugs.gnu.org
> Date: Tue, 24 Sep 2024 14:34:28 +0200
> X-Spam-Status: No
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Óscar Fuentes <ofv@wanadoo.es>
> >> Date: Tue, 24 Sep 2024 00:29:37 +0200
> >> 
> >> BTW, #63752 was filed about the observable problems when Emacs is built
> >> with _FORTIFY_SOURCE on mingw-w64, which documents an ensuing effort to
> >> try to identify the cause, unsuccessfully.
> >
> > I don't think I follow.  Are you saying that these two bugs should be
> > merged, because solving this problem with 'read' solves also the
> > problems reported in bug#63752?  Or are there other problems with
> > _FORTIFY_SOURCE that are still left unresolved if the issue with
> > 'read' is fixed?
> 
> #63752 describes the simptons and tries to identify the cause, this bug
> (#73444) provides the cause and discusses a possible fix, hopefully one
> that covers the general case and not just `read'.

The fix posted here affects only 'read', unless I'm missing
something...

> I was not aware of the existence of #63752 when I created this bug. As
> what to do with #63752, I have no preferences.

That's okay.  If you tell that fixing the issue with 'read' solves the
problem described in bug#63752, I will merge them.  But if some
problems caused by _FORTIFY_SOURCE still remain after fixing 'read',
then bug#63752 is more general, and I think they should remain
separate, so that when we close this one, the other one stays open.

bug#73444: 30.0.50; mingw-w64: Emacs uses CRT's `read` when _FORTIFY_SOURCE > 0

[Óscar Fuentes]

Eli Zaretskii <eliz@gnu.org> writes:

>> #63752 describes the simptons and tries to identify the cause, this bug
>> (#73444) provides the cause and discusses a possible fix, hopefully one
>> that covers the general case and not just `read'.
>
> The fix posted here affects only 'read', unless I'm missing
> something...

Well, if we could think of a general fix that prevents a potential
problem with the other functions that Emacs redefines, I could try to
implement it.

>> I was not aware of the existence of #63752 when I created this bug. As
>> what to do with #63752, I have no preferences.
>
> That's okay.  If you tell that fixing the issue with 'read' solves the
> problem described in bug#63752, I will merge them.  But if some
> problems caused by _FORTIFY_SOURCE still remain after fixing 'read',
> then bug#63752 is more general, and I think they should remain
> separate, so that when we close this one, the other one stays open.

AFAIK the workaround on this bug report fixes #63752. CCing the original
bug reporter, just in case.

bug#73444: 30.0.50; mingw-w64: Emacs uses CRT's `read` when _FORTIFY_SOURCE > 0

[Eli Zaretskii]

> From: Óscar Fuentes <ofv@wanadoo.es>
> Cc: 73444@debbugs.gnu.org, Cyril Arnould <cyril.arnould@outlook.com>
> Date: Tue, 24 Sep 2024 16:06:13 +0200
> X-Spam-Status: No
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> #63752 describes the simptons and tries to identify the cause, this bug
> >> (#73444) provides the cause and discusses a possible fix, hopefully one
> >> that covers the general case and not just `read'.
> >
> > The fix posted here affects only 'read', unless I'm missing
> > something...
> 
> Well, if we could think of a general fix that prevents a potential
> problem with the other functions that Emacs redefines, I could try to
> implement it.

AFAIR, that's a very large job, because each redirected function has
its own story.  If you have time to work on that, please do, but I'd
consider this low priority unless there are known problems with
specific functions we redirect.

> >> I was not aware of the existence of #63752 when I created this bug. As
> >> what to do with #63752, I have no preferences.
> >
> > That's okay.  If you tell that fixing the issue with 'read' solves the
> > problem described in bug#63752, I will merge them.  But if some
> > problems caused by _FORTIFY_SOURCE still remain after fixing 'read',
> > then bug#63752 is more general, and I think they should remain
> > separate, so that when we close this one, the other one stays open.
> 
> AFAIK the workaround on this bug report fixes #63752. CCing the original
> bug reporter, just in case.

Thanks, I will wait for a while and merge if no objections or comments
to the contrary arise.

bug#73444: 30.0.50; mingw-w64: Emacs uses CRT's `read` when _FORTIFY_SOURCE > 0

[Eli Zaretskii]

> From: Óscar Fuentes <ofv@wanadoo.es>
> Date: Tue, 24 Sep 2024 00:15:11 +0200
> 
> 
> On Onssee
> When the macro _FORTIFY_SOURCE > 0, mingw-64 provides an inline
> definition of `read` on io.h:
> 
> __mingw_bos_extern_ovr
> int read(int __fh, void * __dst, unsigned int __n)
> {
>   return _read(__fh, __dst, __n);
> }

Isn't that a bug in MinGW64's io.h?  They should have used

  __mingw_bos_extern_ovr
  int (read)(int __fh, void * __dst, unsigned int __n)
  {
    return _read(__fh, __dst, __n);
  }

Then we could modify the macro slightly as follows:

  #define read(h,d,n) sys_read(h,d,n)

and avoid the problem.  The above is how you protect your functions
from being interpreted as macro invocations.  But I guess this is
water under the bridge now?

> A hack that avoids this consists on doing something like:
> 
> #define read dummy_read
> // etc
> #include <io.h>
> // etc
> #undef read
> #define read sys_read
> int sys_read (int, char *, unsigned int);

This indeed needs the prototype for sys_read, which is less desirable,
because we lose the ability to have the prototype exactly match io.h
without knowing what's in io.h.  But I guess there's no better way,
sigh...

> or simpler but untested:
> 
> #define _read sys_read
> // etc
> #include <io.h>
> // etc

That's simply wrong: we do NOT want to replace the Microsoft '_read',
we want to replace the Posix 'read' where it is used by Emacs.

> Either way it is necessary to condition the hack on the value of
> _FORTIFY_SOURCE.

We could do that unconditionally, no?

Does the MinGW64 build with _FORTIFY_SOURCE work, after taking
care of that?

> More generally, the way Emacs/NT overrides the CRT functions is
> susceptible to break anytime upstream does something like, this case,
> adding an inline definition, or some other unanticipated change. AFAIK
> the C standard says that precisely what Emacs is doing incurs on
> undefined behavior.
> 
> Any ideas about how to future-proof the solution for this problem?

Not elegant ones, no.  We are redirecting Posix functions to our
implementations where Emacs expects them to do something the MS
runtime doesn't, and we don't want to reproduce all the stuff in the
system headers that is related to those functions, including specific
data types, symbols, etc.

> BTW, the initial bug report for this was in March 2023 and only today
> was succesfully analyzed (1) This gives an idea of how problematic this
> practice of redefining standard functions can be.

Trying to make Emacs work well on MS-Windows is problematic in itself,
so we shouldn't be surprised it uses some "unconventional" techniques.

bug#73444: 30.0.50; mingw-w64: Emacs uses CRT's `read` when _FORTIFY_SOURCE > 0

[Óscar Fuentes]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Óscar Fuentes <ofv@wanadoo.es>
>> Date: Tue, 24 Sep 2024 00:15:11 +0200
>> 
>> 
>> On Onssee
>> When the macro _FORTIFY_SOURCE > 0, mingw-64 provides an inline
>> definition of `read` on io.h:
>> 
>> __mingw_bos_extern_ovr
>> int read(int __fh, void * __dst, unsigned int __n)
>> {
>>   return _read(__fh, __dst, __n);
>> }
>
> Isn't that a bug in MinGW64's io.h?  They should have used
>
>   __mingw_bos_extern_ovr
>   int (read)(int __fh, void * __dst, unsigned int __n)
>   {
>     return _read(__fh, __dst, __n);
>   }
>
> Then we could modify the macro slightly as follows:
>
>   #define read(h,d,n) sys_read(h,d,n)
>
> and avoid the problem.  The above is how you protect your functions
> from being interpreted as macro invocations.

Well, AFAIK we are not supposed to #define names on the CRT namespace.

>  But I guess this is water under the bridge now?

Yep, so discussing this is moot.

>> A hack that avoids this consists on doing something like:
>> 
>> #define read dummy_read
>> // etc
>> #include <io.h>
>> // etc
>> #undef read
>> #define read sys_read
>> int sys_read (int, char *, unsigned int);
>
> This indeed needs the prototype for sys_read, which is less desirable,
> because we lose the ability to have the prototype exactly match io.h
> without knowing what's in io.h.  But I guess there's no better way,
> sigh...
>
>> or simpler but untested:
>> 
>> #define _read sys_read
>> // etc
>> #include <io.h>
>> // etc
>
> That's simply wrong: we do NOT want to replace the Microsoft '_read',
> we want to replace the Posix 'read' where it is used by Emacs.

Ok.

>> Either way it is necessary to condition the hack on the value of
>> _FORTIFY_SOURCE.
>
> We could do that unconditionally, no?
>
> Does the MinGW64 build with _FORTIFY_SOURCE work, after taking
> care of that?

I tested that Emacs/MinGW64 + _FORTIFY_SOURCE works with the

#define read dummy_read

hack. Once we put the prototype for sys_read on ms-w32.h, maybe there is
no need to put a conditional on _FORTIFY_SOURCE as well. I can check
that.

>> More generally, the way Emacs/NT overrides the CRT functions is
>> susceptible to break anytime upstream does something like, this case,
>> adding an inline definition, or some other unanticipated change. AFAIK
>> the C standard says that precisely what Emacs is doing incurs on
>> undefined behavior.
>> 
>> Any ideas about how to future-proof the solution for this problem?
>
> Not elegant ones, no.  We are redirecting Posix functions to our
> implementations where Emacs expects them to do something the MS
> runtime doesn't, and we don't want to reproduce all the stuff in the
> system headers that is related to those functions, including specific
> data types, symbols, etc.
>
>> BTW, the initial bug report for this was in March 2023 and only today
>> was succesfully analyzed (1) This gives an idea of how problematic this
>> practice of redefining standard functions can be.
>
> Trying to make Emacs work well on MS-Windows is problematic in itself,
> so we shouldn't be surprised it uses some "unconventional" techniques.

Indeed. I was hoping for a trick from some of you C wizards.

So, ok to install the workaround? On which branch?

1 file changed, 6 insertions(+), 1 deletion(-)
nt/inc/ms-w32.h | 7 ++++++-

modified   nt/inc/ms-w32.h
@@ -257,7 +257,7 @@ extern void w32_reset_stack_overflow_guard (void);
 #define link    sys_link
 #define localtime sys_localtime
 #undef read
-#define read    sys_read
+#define read    unwanted_read // Override the CRT read, see #73444
 #define rename  sys_rename
 #define rmdir   sys_rmdir
 #define select  sys_select
@@ -380,6 +380,11 @@ extern struct tm *localtime_r (time_t const * restrict, struct tm * restrict);
 #define fileno	  _fileno
 #endif
 
+// Here we override CRT read with our own, see #73444
+#undef read
+#define read    sys_read
+int sys_read (int, char *, unsigned int);
+
 /* Defines that we need that aren't in the standard signal.h.  */
 #define SIGHUP  1               /* Hang up */
 #define SIGQUIT 3               /* Quit process */

bug#73444: 30.0.50; mingw-w64: Emacs uses CRT's `read` when _FORTIFY_SOURCE > 0

[Eli Zaretskii]

> From: Óscar Fuentes <ofv@wanadoo.es>
> Cc: 73444@debbugs.gnu.org
> Date: Tue, 24 Sep 2024 14:55:56 +0200
> 
> I tested that Emacs/MinGW64 + _FORTIFY_SOURCE works with the
> 
> #define read dummy_read
> 
> hack. Once we put the prototype for sys_read on ms-w32.h, maybe there is
> no need to put a conditional on _FORTIFY_SOURCE as well. I can check
> that.

Yes, I'd like to avoid the _FORTIFY_SOURCE conditional, so that this
code is always used.

> So, ok to install the workaround? On which branch?

On master, please.

> @@ -257,7 +257,7 @@ extern void w32_reset_stack_overflow_guard (void);
>  #define link    sys_link
>  #define localtime sys_localtime
>  #undef read
> -#define read    sys_read
> +#define read    unwanted_read // Override the CRT read, see #73444
>  #define rename  sys_rename
>  #define rmdir   sys_rmdir
>  #define select  sys_select
> @@ -380,6 +380,11 @@ extern struct tm *localtime_r (time_t const * restrict, struct tm * restrict);
>  #define fileno	  _fileno
>  #endif
>  
> +// Here we override CRT read with our own, see #73444
> +#undef read
> +#define read    sys_read
> +int sys_read (int, char *, unsigned int);
> +
>  /* Defines that we need that aren't in the standard signal.h.  */
>  #define SIGHUP  1               /* Hang up */
>  #define SIGQUIT 3               /* Quit process */

But please don't use "//" as comment style, and please don't forget
mentioning the bug number in the commit log message.

Thanks.

bug#73444: 30.0.50; mingw-w64: Emacs uses CRT's `read` when _FORTIFY_SOURCE > 0

[Óscar Fuentes]

Cyril Arnould <cyril.arnould@outlook.com> writes:

> I've tested the workaround applied to master and AFAICT it fixes
> #63752. Emacs
> was pretty much unusable throughout in the bugged build, with the workaround
> it behaves normally.

Thanks for testing. I will apply the workaround with the adjustments
requested by Eli.

bug#73444: 30.0.50; mingw-w64: Emacs uses CRT's `read` when _FORTIFY_SOURCE > 0

[Eli Zaretskii]

merge 73444 63752
thanks

> Date: Wed, 25 Sep 2024 00:25:23 +0200
> Cc: Eli Zaretskii <eliz@gnu.org>
> From: Cyril Arnould <cyril.arnould@outlook.com>
> 
>  > AFAIK the workaround on this bug report fixes #63752. CCing the original
>  > bug reporter, just in case.
> 
> I've tested the workaround applied to master and AFAICT it fixes #63752. 
> Emacs
> was pretty much unusable throughout in the bugged build, with the workaround
> it behaves normally.

Thanks, I'm therefore merging these two bugs.

P.S. Please in the future keep the bug address on the CC list.