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

["Óscar Fuentes" <ofv@wanadoo.es>]

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 */