unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
* bug#63752: 28.2; GCC 13.1 breaks Emacs subprocess handling when built with -D_FORTIFY_SOURCE
@ 2023-05-27 12:57 Cyril Arnould
  2023-05-27 13:42 ` Eli Zaretskii
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Cyril Arnould @ 2023-05-27 12:57 UTC (permalink / raw)
  To: 63752

[-- Attachment #1: Type: text/plain, Size: 5553 bytes --]

First of all, I'm on Windows 10, using the MINGW64 MSYS2 environment, so
this issue might be specific to MinGW. Note: While I'm using a UCRT
build of Emacs to generate this report, the problem is present in the
MinGW GCC build as well. Emacs has some weird behaviour regarding
subprocess handling (?) when compiled with
"-D_FORTIFY_SOURCE=1". Setting the value to 2 causes the same issue, I
haven't tested setting it to 3. I believe this is the why the MSYS2
Emacs builds have been broken for a while now:

https://github.com/msys2/MINGW-packages/issues/16190
https://github.com/msys2/MINGW-packages/issues/16375
https://github.com/msys2/MINGW-packages/issues/16413

I've managed to reproduce the issue when building from the emacs
sources as follows:

git clone https://git.savannah.gnu.org/git/emacs.git
cd emacs
git checkout emacs-28.2
./autogen.sh
export CFLAGS='-g3 -O2 -gdwarf-2 -Wp,-D_FORTIFY_SOURCE=1'
./configure
make

"-g3 -O2 -gdwarf-2" were the default compilation options. The build
terminates fine but the executable has problems. Executing "dir" via
async-shell-command doesn't terminate properly, for example:

./src/emacs -Q --eval '(async-shell-command "dir")'

Trying to close the window results in Emacs saying there are active
processes, which there shouldn't be. Other symptoms are described in the
linked Github issues.

In the example above, I built Emacs from the emacs-28.2 tag, but I also
managed to reproduce the issue in the emacs-28, emacs-29 and master
branches.



In GNU Emacs 28.2 (build 2, x86_64-w64-mingw32)
of 2023-05-27 built on ZUERILL
Repository revision: f31980c83aee3840ee90688447198a37a14f7aaf
Repository branch: emacs-28.2
Windowing system distributor 'Microsoft Corp.', version 10.0.19044
System Description: Microsoft Windows 10 Pro (v10.0.2009.19044.2965)

Configured using:
'configure --prefix=/ucrt64 --host=x86_64-w64-mingw32
--build=x86_64-w64-mingw32 --with-modules --without-dbus
--without-compress-install --with-native-compilation
'CFLAGS=-march=nocona -msahf -mtune=generic -O2 -pipe
-fstack-protector-strong' CPPFLAGS=-D__USE_MINGW_ANSI_STDIO=1
'LDFLAGS=-pipe -lpthread''

Configured features:
ACL GIF GMP GNUTLS HARFBUZZ JPEG JSON LCMS2 LIBXML2 MODULES NATIVE_COMP
NOTIFY W32NOTIFY PDUMPER PNG RSVG SOUND THREADS TIFF TOOLKIT_SCROLL_BARS
XPM ZLIB

Important settings:
  value of $LANG: en_GB
  locale-coding-system: cp1252

Major mode: Lisp Interaction

Minor modes in effect:
  display-line-numbers-mode: t
  override-global-mode: t
  delete-selection-mode: t
  tabbar-mwheel-mode: t
  tabbar-mode: t
  global-so-long-mode: t
  global-flycheck-mode: t
  flycheck-mode: t
  cua-mode: t
  company-tng-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  size-indication-mode: t
  column-number-mode: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
~/.lisp/load/vhdl-mode hides c:/msys64/ucrt64/share/emacs/28.2/lisp/progmodes/vhdl-mode

Features:
(shadow sort mail-extr emacsbug message rmc puny dired dired-loaddefs
rfc822 mml mml-sec epa derived epg rfc6068 epg-config gnus-util rmail
rmail-loaddefs time-date mm-decode mm-bodies mm-encode mail-parse
rfc2231 mailabbrev gmm-utils mailheader sendmail rfc2047 rfc2045
ietf-drums mm-util mail-prsvr mail-utils display-line-numbers cl-extra
use-package use-package-ensure use-package-delight use-package-diminish
use-package-bind-key bind-key use-package-core server delsel tabbar
easy-mmode grep compile text-property-search comint ring so-long
flycheck ansi-color find-func help-mode rx dash cua-base company-tng
company edmacro kmacro pcase cus-load tex-site info package browse-url
url url-proxy url-privacy url-expand url-methods url-history url-cookie
url-domsuf url-util mailcap url-handlers url-parse auth-source cl-seq
eieio eieio-core cl-macs eieio-loaddefs password-cache json subr-x map
url-vars seq byte-opt gv bytecomp byte-compile cconv cl-loaddefs cl-lib
iso-transl tooltip eldoc paren electric uniquify ediff-hook vc-hooks
lisp-float-type elisp-mode mwheel dos-w32 ls-lisp disp-table
term/w32-win w32-win w32-vars term/common-win tool-bar dnd fontset image
regexp-opt fringe tabulated-list replace newcomment text-mode lisp-mode
prog-mode register page tab-bar menu-bar rfn-eshadow isearch easymenu
timer select scroll-bar mouse jit-lock font-lock syntax font-core
term/tty-colors frame minibuffer cl-generic cham georgian utf-8-lang
misc-lang vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms
cp51932 hebrew greek romanian slovak czech european ethiopic indian
cyrillic chinese composite emoji-zwj charscript charprop case-table
epa-hook jka-cmpr-hook help simple abbrev obarray cl-preloaded nadvice
button loaddefs faces cus-face macroexp files window text-properties
overlay sha1 md5 base64 format env code-pages mule custom widget
hashtable-print-readable backquote threads w32notify w32 lcms2 multi-tty
make-network-process native-compile emacs)

Memory information:
((conses 16 161207 53896)
(symbols 48 13207 112)
(strings 32 43518 8562)
(string-bytes 1 1529452)
(vectors 16 23546)
(vector-slots 8 437343 90008)
(floats 8 73 206)
(intervals 56 299 290)
(buffers 992 11))

[-- Attachment #2: Type: text/html, Size: 10678 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* bug#63752: 28.2; GCC 13.1 breaks Emacs subprocess handling when built with -D_FORTIFY_SOURCE
  2023-05-27 12:57 bug#63752: 28.2; GCC 13.1 breaks Emacs subprocess handling when built with -D_FORTIFY_SOURCE Cyril Arnould
@ 2023-05-27 13:42 ` Eli Zaretskii
  2023-05-27 14:32   ` bug#63752: AW: " Cyril Arnould
  2023-06-30 22:41 ` Cyril Arnould
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 9+ messages in thread
From: Eli Zaretskii @ 2023-05-27 13:42 UTC (permalink / raw)
  To: Cyril Arnould; +Cc: 63752

> From: Cyril Arnould <cyril.arnould@outlook.com>
> Date: Sat, 27 May 2023 12:57:44 +0000
> 
> First of all, I'm on Windows 10, using the MINGW64 MSYS2 environment, so
> this issue might be specific to MinGW. Note: While I'm using a UCRT
> build of Emacs to generate this report, the problem is present in the
> MinGW GCC build as well. Emacs has some weird behaviour regarding
> subprocess handling (?) when compiled with
> "-D_FORTIFY_SOURCE=1". Setting the value to 2 causes the same issue, I
> haven't tested setting it to 3. I believe this is the why the MSYS2
> Emacs builds have been broken for a while now:

Thank you for your report.

What happens if you don't use -D_FORTIFY_SOURCE= at all?

I don't know what this option odes for MSYS2 MinGW GCC compiler and
its header files.  Up until now I knew that this option is only used
on GNU/Linux.  In any case, I see no reason for end-user builds to use
it, as it is (AFAIU) intended for developers, to facilitate detection
of problematic code.

> https://github.com/msys2/MINGW-packages/issues/16190
> https://github.com/msys2/MINGW-packages/issues/16375
> https://github.com/msys2/MINGW-packages/issues/16413

I see nothing in these issues that would point out some upstream
problem.  The issues are all related to the Emacs builds produced by
the MSYS2 folks.  So I think the initial investigation should be
theirs, until they identify the code in Emacs which misbehaves.

> In the example above, I built Emacs from the emacs-28.2 tag, but I also
> managed to reproduce the issue in the emacs-28, emacs-29 and master
> branches.

FWIW, Emacs 28 and 29 (and also Emacs 30) builds fine for me with
MinGW, both with and without native-compilation, but I don't use
_FORTIFY_SOURCE (and don't plan on using it any time soon).

Which version of GCC is this, btw?  If it's the latest GCC 13, did you
try downgrading to GCC 12?

Bottom line, I see no evidence yet that this is an upstream problem of
Emacs, and my MinGW builds are all fine.





^ permalink raw reply	[flat|nested] 9+ messages in thread

* bug#63752: AW: bug#63752: 28.2; GCC 13.1 breaks Emacs subprocess handling when built with -D_FORTIFY_SOURCE
  2023-05-27 13:42 ` Eli Zaretskii
@ 2023-05-27 14:32   ` Cyril Arnould
  2023-06-01  7:31     ` András Svraka
  0 siblings, 1 reply; 9+ messages in thread
From: Cyril Arnould @ 2023-05-27 14:32 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 63752@debbugs.gnu.org

[-- Attachment #1: Type: text/plain, Size: 1498 bytes --]

> What happens if you don't use -D_FORTIFY_SOURCE= at all?

I have no problems when not using -D_FORTIFY_SOURCE. It's set to
2 by default in the MSYS2 builds, I figured lowering it to 1
might work better but neither seem to work.

> I see nothing in these issues that would point out some upstream
> problem.  The issues are all related to the Emacs builds produced by
> the MSYS2 folks.  So I think the initial investigation should be
> theirs, until they identify the code in Emacs which misbehaves.

I understand. I'll reach out to see how they want to proceed on
their side.

I've also just noticed that the -D_FORTIFY_SOURCE option seems to
have been a rather recent addition to the MSYS2 build options and
it seems like this was only recently even made possible. And the
last successful MSYS2 build of Emacs was before that change.

> Which version of GCC is this, btw?  If it's the latest GCC 13, did you
> try downgrading to GCC 12?

It's GCC 13.1, yes, I haven't tested GCC 12. I don't know how
easy it is to downgrade GCC on MSYS2 though, I assume there's a
lot of dependencies that can go wrong.

> FWIW, Emacs 28 and 29 (and also Emacs 30) builds fine for me with
> MinGW, both with and without native-compilation, but I don't use
> _FORTIFY_SOURCE (and don't plan on using it any time soon).

This makes me wonder if GCC 13.1 Emacs builds with
_FORTIFY_SOURCE are working fine in other operating systems? It's
not something I can easily test, however.

[-- Attachment #2: Type: text/html, Size: 3672 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* bug#63752: 28.2; GCC 13.1 breaks Emacs subprocess handling when built with -D_FORTIFY_SOURCE
  2023-05-27 14:32   ` bug#63752: AW: " Cyril Arnould
@ 2023-06-01  7:31     ` András Svraka
  0 siblings, 0 replies; 9+ messages in thread
From: András Svraka @ 2023-06-01  7:31 UTC (permalink / raw)
  To: 63752@debbugs.gnu.org; +Cc: Eli Zaretskii, Cyril Arnould

Dear all,

> On 2023. May 27., at 16:32, Cyril Arnould <cyril.arnould@outlook.com> wrote:
>  > Which version of GCC is this, btw?  If it's the latest GCC 13, did you
> > try downgrading to GCC 12?
>  It's GCC 13.1, yes, I haven't tested GCC 12. I don't know how
> easy it is to downgrade GCC on MSYS2 though, I assume there's a
> lot of dependencies that can go wrong.

I tried downgrading to GCC 12.2 in MSY2 and the problem exist there as well.

> > FWIW, Emacs 28 and 29 (and also Emacs 30) builds fine for me with
> > MinGW, both with and without native-compilation, but I don't use
> > _FORTIFY_SOURCE (and don't plan on using it any time soon).
>  This makes me wonder if GCC 13.1 Emacs builds with
> _FORTIFY_SOURCE are working fine in other operating systems? It's
> not something I can easily test, however.

Arch Linux also uses _FORTIFY_SOURCE and AFAIU MSYS2’s motivation was to follow them [1]. I did not test it though but couldn’t find related bug reports.

[1]: https://github.com/msys2/MSYS2-packages/pull/3222






^ permalink raw reply	[flat|nested] 9+ messages in thread

* bug#63752: 28.2; GCC 13.1 breaks Emacs subprocess handling when built with -D_FORTIFY_SOURCE
  2023-05-27 12:57 bug#63752: 28.2; GCC 13.1 breaks Emacs subprocess handling when built with -D_FORTIFY_SOURCE Cyril Arnould
  2023-05-27 13:42 ` Eli Zaretskii
@ 2023-06-30 22:41 ` Cyril Arnould
  2023-07-01  6:40   ` Eli Zaretskii
  2023-07-05 20:23 ` Cyril Arnould
  2023-07-06 19:28 ` Cyril Arnould
  3 siblings, 1 reply; 9+ messages in thread
From: Cyril Arnould @ 2023-06-30 22:41 UTC (permalink / raw)
  To: eliz@gnu.org; +Cc: András Svraka, 63752@debbugs.gnu.org


[-- Attachment #1.1: Type: text/plain, Size: 572 bytes --]

I've found that I can narrow it down similar to bug #63365: As long as I
don't compile src/sysdep.c with -D_FORTIFY_SOURCE, the resulting
binaries seem to behave normally. Does that maybe suggest an upstream
problem? I compiled it as follows:

git clean -xdf
git checkout emacs-28.2
./autogen.sh
./configure
cd src
make sysdep.o CFLAGS='-g3 -O2 -gdwarf-2 -Wp,-D_FORTIFY_SOURCE=2'
make sysdep.o -W sysdep.c
cd ..
make CFLAGS='-g3 -O2 -gdwarf-2 -Wp,-D_FORTIFY_SOURCE=2'

I've attached the objects as well as their dumps in case anyone wants to
take a look.

[-- Attachment #1.2: Type: text/html, Size: 2162 bytes --]

[-- Attachment #2: sysdep-diff.tar.gz --]
[-- Type: application/x-gzip, Size: 866575 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* bug#63752: 28.2; GCC 13.1 breaks Emacs subprocess handling when built with -D_FORTIFY_SOURCE
  2023-06-30 22:41 ` Cyril Arnould
@ 2023-07-01  6:40   ` Eli Zaretskii
  0 siblings, 0 replies; 9+ messages in thread
From: Eli Zaretskii @ 2023-07-01  6:40 UTC (permalink / raw)
  To: Cyril Arnould; +Cc: svraka.andras, 63752

> From: Cyril Arnould <cyril.arnould@outlook.com>
> CC: "63752@debbugs.gnu.org" <63752@debbugs.gnu.org>,
> 	András Svraka <svraka.andras@gmail.com>
> Date: Fri, 30 Jun 2023 22:41:26 +0000
> 
> I've found that I can narrow it down similar to bug #63365: As long as I
> don't compile src/sysdep.c with -D_FORTIFY_SOURCE, the resulting
> binaries seem to behave normally. Does that maybe suggest an upstream
> problem? I compiled it as follows:
> 
> git clean -xdf
> git checkout emacs-28.2
> ./autogen.sh
> ./configure
> cd src
> make sysdep.o CFLAGS='-g3 -O2 -gdwarf-2 -Wp,-D_FORTIFY_SOURCE=2'
> make sysdep.o -W sysdep.c
> cd ..
> make CFLAGS='-g3 -O2 -gdwarf-2 -Wp,-D_FORTIFY_SOURCE=2'
> 
> I've attached the objects as well as their dumps in case anyone wants to
> take a look.

Thanks, but the differences are too large to figure out what causes
this.  It sounds like -D_FORTIFY_SOURCE=2 wraps and/or replaces many
library functions with special variants, and effect of that is
unclear, because the implementation of those wrappers is elsewhere
(probably in some library linked into the executable under that
option?).  E.g., by just looking at the diffs, it sounds like the
-D_FORTIFY_SOURCE=2 code doesn't call 'open' to open the null device??

Since you've already established sysdep.c alone is the culprit, I
suggest to narrow this.  Create a separate file sysdep1.c, move to it
the first portion of sysdep.c which includes everything before
serial_open, and build Emacs with these two parts (you'd need to add
sysdep1.o to base_obj in src/Makefile).  Once you have such an Emacs
successfully built and reproduce the problem, rebuild by compiling
sysdep1.c without -D_FORTIFY_SOURCE=2, and see if the problem goes
away.  If it does, bisect sysdep1.c by moving parts of it back to
sysdep.c, until you identify the smallest part of sysdep.c that needs
to be compiled with -D_FORTIFY_SOURCE=2 to reproduce the problem.
Then we will have to examine the effect of -D_FORTIFY_SOURCE=2 on that
smallest part, and see if we can figure this out.

This is a lot of work, so kudos if you are motivated to go ahead and
do it.  From my POV, the -D_FORTIFY_SOURCE=2 build is currently
problematic on Windows and therefore not supported by the upstream
project.  (IMNSHO, it is also wrong to use this in production builds
of programs such as Emacs, but that's me, and I know others disagree.)
My advice to MSYS2 folks at this time is to stop building Emacs that
way until the problem is resolved.





^ permalink raw reply	[flat|nested] 9+ messages in thread

* bug#63752: 28.2; GCC 13.1 breaks Emacs subprocess handling when built with -D_FORTIFY_SOURCE
  2023-05-27 12:57 bug#63752: 28.2; GCC 13.1 breaks Emacs subprocess handling when built with -D_FORTIFY_SOURCE Cyril Arnould
  2023-05-27 13:42 ` Eli Zaretskii
  2023-06-30 22:41 ` Cyril Arnould
@ 2023-07-05 20:23 ` Cyril Arnould
  2023-07-06  5:28   ` Eli Zaretskii
  2023-07-06 19:28 ` Cyril Arnould
  3 siblings, 1 reply; 9+ messages in thread
From: Cyril Arnould @ 2023-07-05 20:23 UTC (permalink / raw)
  To: eliz@gnu.org; +Cc: svraka.andras@gmail.com, 63752@debbugs.gnu.org


[-- Attachment #1.1: Type: text/plain, Size: 2894 bytes --]

> Since you've already established sysdep.c alone is the culprit, I
> suggest to narrow this.  Create a separate file sysdep1.c, move to it
> the first portion of sysdep.c which includes everything before
> serial_open, and build Emacs with these two parts (you'd need to add
> sysdep1.o to base_obj in src/Makefile).  Once you have such an Emacs
> successfully built and reproduce the problem, rebuild by compiling
> sysdep1.c without -D_FORTIFY_SOURCE=2, and see if the problem goes
> away.  If it does, bisect sysdep1.c by moving parts of it back to
> sysdep.c, until you identify the smallest part of sysdep.c that needs
> to be compiled with -D_FORTIFY_SOURCE=2 to reproduce the problem.
> Then we will have to examine the effect of -D_FORTIFY_SOURCE=2 on that
> smallest part, and see if we can figure this out.

Thanks for the pointers! Following your advice, the first sysdep1.c with
code up until serial_open was working quite quickly. On a hunch, I've
singled out the emacs_intr_read function and got lucky: If I only put
the emacs_intr_read, emacs_read and emacs_read_quit functions into
sysdep1.c, the problems go away if sysdep1.o is compiled without
-D_FORTIFY_SOURCE=2.

Aside from those functions, sysdep1.c also contains all of the includes
of the original file; I figured it doesn't make much sense to narrow
those down further. I also had to add the MAX_RW_COUNT
definition. There's a patch attached; it can be tested as follows:

git clean -xdf
git checkout emacs-28.2
patch -p1 -i 0001-Single-out-emacs_read-as-culprit.patch
./autogen.sh
./configure
make CFLAGS='-g3 -O2 -gdwarf-2 -Wp,-D_FORTIFY_SOURCE=2'
# The following command will result in an Emacs window where the
# asynchronous process doesn't terminate:
./src/emacs -Q --eval '(async-shell-command "dir")'
cd src
make sysdep1.o -W sysdep1.c
cd ..
make CFLAGS='-g3 -O2 -gdwarf-2 -Wp,-D_FORTIFY_SOURCE=2'
# This time the process will terminate:
./src/emacs -Q --eval '(async-shell-command "dir")'

I've also attached another archive with the objects, dumps and
diff. This time it seems like the entire file is different. Is it
possible that GCC messes up with the read function that gets defined to
sys_read at the top?

> This is a lot of work, so kudos if you are motivated to go ahead and
> do it.  From my POV, the -D_FORTIFY_SOURCE=2 build is currently
> problematic on Windows and therefore not supported by the upstream
> project.  (IMNSHO, it is also wrong to use this in production builds
> of programs such as Emacs, but that's me, and I know others disagree.)

Fully understand, I really appreciate you helping despite the problem
being of low priority.

> My advice to MSYS2 folks at this time is to stop building Emacs that
> way until the problem is resolved.

The current release of Emacs on MSYS2 is indeed built without
-D_FORTIFY_SOURCE.

[-- Attachment #1.2: Type: text/html, Size: 5761 bytes --]

[-- Attachment #2: 0001-Single-out-emacs_read-as-culprit.patch --]
[-- Type: application/octet-stream, Size: 6977 bytes --]

From 67522e3f7728b7d6c833f813f6d8c8390e3f0e3e Mon Sep 17 00:00:00 2001
From: Cyril Arnould <cyril.arnould@outlook.com>
Date: Wed, 5 Jul 2023 19:08:09 +0200
Subject: [PATCH] Single out emacs_read as culprit

Compiling everything but sysdep1.c with -D_FORTIFY_SOURCE=2 results in
a working binary
---
 src/Makefile.in |   2 +-
 src/sysdep.c    |  41 ------------
 src/sysdep1.c   | 166 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 167 insertions(+), 42 deletions(-)
 create mode 100644 src/sysdep1.c

diff --git a/src/Makefile.in b/src/Makefile.in
index 29e1513ab5f..3bd17b8f6b5 100644
--- a/src/Makefile.in
+++ b/src/Makefile.in
@@ -394,7 +394,7 @@ .m.o:
 base_obj = dispnew.o frame.o scroll.o xdisp.o menu.o $(XMENU_OBJ) window.o \
 	charset.o coding.o category.o ccl.o character.o chartab.o bidi.o \
 	$(CM_OBJ) term.o terminal.o xfaces.o $(XOBJ) $(GTK_OBJ) $(DBUS_OBJ) \
-	emacs.o keyboard.o macros.o keymap.o sysdep.o \
+	emacs.o keyboard.o macros.o keymap.o sysdep.o sysdep1.o \
 	bignum.o buffer.o filelock.o insdel.o marker.o \
 	minibuf.o fileio.o dired.o \
 	cmds.o casetab.o casefiddle.o indent.o search.o regex-emacs.o undo.o \
diff --git a/src/sysdep.c b/src/sysdep.c
index 72be25f6610..ad344ef44f0 100644
--- a/src/sysdep.c
+++ b/src/sysdep.c
@@ -2474,47 +2474,6 @@ verify (MAX_RW_COUNT <= INT_MAX);
 verify (MAX_RW_COUNT <= UINT_MAX);
 #endif
 
-/* Read from FD to a buffer BUF with size NBYTE.
-   If interrupted, process any quits and pending signals immediately
-   if INTERRUPTIBLE, and then retry the read unless quitting.
-   Return the number of bytes read, which might be less than NBYTE.
-   On error, set errno to a value other than EINTR, and return -1.  */
-static ptrdiff_t
-emacs_intr_read (int fd, void *buf, ptrdiff_t nbyte, bool interruptible)
-{
-  /* No caller should ever pass a too-large size to emacs_read.  */
-  eassert (nbyte <= MAX_RW_COUNT);
-
-  ssize_t result;
-
-  do
-    {
-      if (interruptible)
-	maybe_quit ();
-      result = read (fd, buf, nbyte);
-    }
-  while (result < 0 && errno == EINTR);
-
-  return result;
-}
-
-/* Read from FD to a buffer BUF with size NBYTE.
-   If interrupted, retry the read.  Return the number of bytes read,
-   which might be less than NBYTE.  On error, set errno to a value
-   other than EINTR, and return -1.  */
-ptrdiff_t
-emacs_read (int fd, void *buf, ptrdiff_t nbyte)
-{
-  return emacs_intr_read (fd, buf, nbyte, false);
-}
-
-/* Like emacs_read, but also process quits and pending signals.  */
-ptrdiff_t
-emacs_read_quit (int fd, void *buf, ptrdiff_t nbyte)
-{
-  return emacs_intr_read (fd, buf, nbyte, true);
-}
-
 /* Write to FILEDES from a buffer BUF with size NBYTE, retrying if
    interrupted or if a partial write occurs.  Process any quits
    immediately if INTERRUPTIBLE is positive, and process any pending
diff --git a/src/sysdep1.c b/src/sysdep1.c
new file mode 100644
index 00000000000..d39692fd577
--- /dev/null
+++ b/src/sysdep1.c
@@ -0,0 +1,166 @@
+/* Interfaces to system-dependent kernel and library entries.
+   Copyright (C) 1985-1988, 1993-1995, 1999-2022 Free Software
+   Foundation, Inc.
+
+This file is part of GNU Emacs.
+
+GNU Emacs is free software: you can redistribute it and/or modify
+it under the terms of the GNU General Public License as published by
+the Free Software Foundation, either version 3 of the License, or (at
+your option) any later version.
+
+GNU Emacs is distributed in the hope that it will be useful,
+but WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+GNU General Public License for more details.
+
+You should have received a copy of the GNU General Public License
+along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.  */
+
+#include <config.h>
+
+#include <execinfo.h>
+#include "sysstdio.h"
+#ifdef HAVE_PWD_H
+#include <pwd.h>
+#include <grp.h>
+#endif /* HAVE_PWD_H */
+#include <limits.h>
+#include <stdlib.h>
+#include <sys/random.h>
+#include <unistd.h>
+
+#include <c-ctype.h>
+#include <close-stream.h>
+#include <pathmax.h>
+#include <utimens.h>
+
+#include "lisp.h"
+#include "sheap.h"
+#include "sysselect.h"
+#include "blockinput.h"
+
+#ifdef HAVE_LINUX_FS_H
+# include <linux/fs.h>
+# include <sys/syscall.h>
+#endif
+
+#ifdef CYGWIN
+# include <cygwin/fs.h>
+#endif
+
+#if defined DARWIN_OS || defined __FreeBSD__ || defined __OpenBSD__
+# include <sys/sysctl.h>
+#endif
+
+#if defined __OpenBSD__
+# include <sys/proc.h>
+#endif
+
+#ifdef DARWIN_OS
+# include <libproc.h>
+#endif
+
+#ifdef __FreeBSD__
+/* Sparc/ARM machine/frame.h has 'struct frame' which conflicts with Emacs's
+   'struct frame', so rename it.  */
+# define frame freebsd_frame
+# include <sys/user.h>
+# undef frame
+
+# include <math.h>
+#endif
+
+#ifdef HAVE_SOCKETS
+#include <sys/socket.h>
+#include <netdb.h>
+#endif /* HAVE_SOCKETS */
+
+#ifdef WINDOWSNT
+#define read sys_read
+#define write sys_write
+#ifndef STDERR_FILENO
+#define STDERR_FILENO fileno(GetStdHandle(STD_ERROR_HANDLE))
+#endif
+#include "w32.h"
+#endif /* WINDOWSNT */
+
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <errno.h>
+
+/* Get SI_SRPC_DOMAIN, if it is available.  */
+#ifdef HAVE_SYS_SYSTEMINFO_H
+#include <sys/systeminfo.h>
+#endif
+
+#ifdef MSDOS	/* Demacs 1.1.2 91/10/20 Manabu Higashida, MW Aug 1993 */
+#include "msdos.h"
+#endif
+
+#include <sys/param.h>
+#include <sys/file.h>
+#include <fcntl.h>
+
+#include "syssignal.h"
+#include "systime.h"
+#include "systty.h"
+#include "syswait.h"
+
+#ifdef HAVE_SYS_RESOURCE_H
+# include <sys/resource.h>
+#endif
+
+#ifdef HAVE_SYS_UTSNAME_H
+# include <sys/utsname.h>
+# include <memory.h>
+#endif
+
+#include "keyboard.h"
+#include "frame.h"
+#include "termhooks.h"
+#include "termchar.h"
+#include "termopts.h"
+#include "process.h"
+#include "cm.h"
+
+#ifndef MAX_RW_COUNT
+#define MAX_RW_COUNT (INT_MAX >> 18 << 18)
+#endif
+
+/* Read from FD to a buffer BUF with size NBYTE.
+   If interrupted, process any quits and pending signals immediately
+   if INTERRUPTIBLE, and then retry the read unless quitting.
+   Return the number of bytes read, which might be less than NBYTE.
+   On error, set errno to a value other than EINTR, and return -1.  */
+static ptrdiff_t
+emacs_intr_read (int fd, void *buf, ptrdiff_t nbyte, bool interruptible)
+{
+  /* No caller should ever pass a too-large size to emacs_read.  */
+  eassert (nbyte <= MAX_RW_COUNT);
+
+  ssize_t result;
+
+  do
+    {
+      if (interruptible)
+	maybe_quit ();
+      result = read (fd, buf, nbyte);
+    }
+  while (result < 0 && errno == EINTR);
+
+  return result;
+}
+
+ptrdiff_t
+emacs_read (int fd, void *buf, ptrdiff_t nbyte)
+{
+  return emacs_intr_read (fd, buf, nbyte, false);
+}
+
+/* Like emacs_read, but also process quits and pending signals.  */
+ptrdiff_t
+emacs_read_quit (int fd, void *buf, ptrdiff_t nbyte)
+{
+  return emacs_intr_read (fd, buf, nbyte, true);
+}
-- 
2.36.0.windows.1


[-- Attachment #3: sysdep1-diff.tar.gz --]
[-- Type: application/x-gzip, Size: 609882 bytes --]

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* bug#63752: 28.2; GCC 13.1 breaks Emacs subprocess handling when built with -D_FORTIFY_SOURCE
  2023-07-05 20:23 ` Cyril Arnould
@ 2023-07-06  5:28   ` Eli Zaretskii
  0 siblings, 0 replies; 9+ messages in thread
From: Eli Zaretskii @ 2023-07-06  5:28 UTC (permalink / raw)
  To: Cyril Arnould, Andrea Corallo; +Cc: svraka.andras, 63752

> From: Cyril Arnould <cyril.arnould@outlook.com>
> CC: "63752@debbugs.gnu.org" <63752@debbugs.gnu.org>, "svraka.andras@gmail.com"
> 	<svraka.andras@gmail.com>
> Date: Wed, 5 Jul 2023 20:23:33 +0000
> 
> Thanks for the pointers! Following your advice, the first sysdep1.c with
> code up until serial_open was working quite quickly. On a hunch, I've
> singled out the emacs_intr_read function and got lucky: If I only put
> the emacs_intr_read, emacs_read and emacs_read_quit functions into
> sysdep1.c, the problems go away if sysdep1.o is compiled without
> -D_FORTIFY_SOURCE=2.

Thanks, I hope Andrea will have ideas after looking at the
differences.

There are several symbols in the diffs which are used only in the
-D_FORTIFY_SOURCE=2 code, whose precise meanings are unclear to me.
Those are the declaration qualifier __mingw_bos_extern_ovr and the
functions __mingw_bos_ptr_chk_warn and __mingw_call__read.

Under __MINGW_FORTIFY_LEVEL > 0 __mingw_bos_extern_ovr is defined like
this:

  #  define __mingw_bos_extern_ovr extern __inline__ __cdecl \
       __attribute__((__always_inline__, __gnu_inline__)) \
       __mingw_attribute_artificial

Otherwise, it's defined to nothing.  The above just controls the
inlining, but that the 'artificial' attribute is described like this
in the GCC manual:

  'artificial'
       This attribute is useful for small inline wrappers that if possible
       should appear during debugging as a unit.  Depending on the debug
       info format it either means marking the function as artificial or
       using the caller location for all instructions within the inlined
       body.

I'm not sure I understand what this means in the context of the issue
we are discussing here.

The other 2 symbols seem to be about checking a pointer and calling
'_read' while checking the pointer to the buffer passed to it.
__mingw_bos_ptr_chk_warn is defined like this under
__MINGW_FORTIFY_LEVEL > 0:

  #  define __mingw_bos_ptr_chk_warn(p, n, maxtype) \
       ((__mingw_bos_known(p) \
       && __builtin_constant_p(__mingw_bos(p, maxtype) < (size_t)(n)) \
       && __mingw_bos(p, maxtype) < (size_t)(n)) \
       ? __mingw_chk_fail_warn() : __mingw_bos_ptr_chk(p, n, maxtype))

And here's __mingw_call__read:

  #define __MINGW_ASM_CRT_CALL(func) __asm__(__STRINGIFY(func))
  _CRTIMP int __cdecl __mingw_call__read(int, void *, unsigned int) __MINGW_ASM_CRT_CALL(_read);

AFAICT, __mingw_chk_fail_warn eventually does this:

  void __cdecl __attribute__((__noreturn__)) __chk_fail(void) {
    static const char msg[] = "*** buffer overflow detected ***: terminated\n";
    write(STDERR_FILENO, msg, strlen(msg));
    if (IsProcessorFeaturePresent(PF_FASTFAIL_AVAILABLE)) {
      __fastfail(FAST_FAIL_RANGE_CHECK_FAILURE);
    } else {
      TerminateProcess(GetCurrentProcess(), STATUS_STACK_BUFFER_OVERRUN);
      __builtin_unreachable();
    }
  }

Again, not sure what this means for the issue at hand.  In particular,
__fastfail emits "int 0x29", see

  https://learn.microsoft.com/en-us/cpp/intrinsics/fastfail?view=msvc-170

> Aside from those functions, sysdep1.c also contains all of the includes
> of the original file; I figured it doesn't make much sense to narrow
> those down further. I also had to add the MAX_RW_COUNT
> definition. There's a patch attached; it can be tested as follows:

Thanks, I hope Andrea will have some ideas.

It's too bad the problem goes away under GDB, because we don't even
know whether the crash is due to some fatal error in the Emacs code,
or due to those extra-checking routines inserted into the code by the
FORTIFY option.  IOW, it could be that the problem happens because the
FORTIFY routines don't understand something Emacs does and consider it
a bug worthy of aborting the program.

Another interesting question is why the problems happen only in Emacs
built with native compilation.  Do we use emacs_read or its callers in
some special ways when they are called from comp.c or comp.el?

Maybe we should ask the MinGW64 folks who implemented the FORTIFY
support for MinGW64 GCC to join this discussion and help us figure out
which part(s) of this puzzle are relevant and why?  Cyril, can you
reach out to them and ask them help us out here?  There are too many
unknowns here, and having someone on board who understands what the
various FORTIFY additions mean and do, and perhaps also how to
selectively disable them (to find the one that really matters) will
definitely help.

Thanks.

P.S. Please keep ANdrea CC'd on this discussion.





^ permalink raw reply	[flat|nested] 9+ messages in thread

* bug#63752: 28.2; GCC 13.1 breaks Emacs subprocess handling when built with -D_FORTIFY_SOURCE
  2023-05-27 12:57 bug#63752: 28.2; GCC 13.1 breaks Emacs subprocess handling when built with -D_FORTIFY_SOURCE Cyril Arnould
                   ` (2 preceding siblings ...)
  2023-07-05 20:23 ` Cyril Arnould
@ 2023-07-06 19:28 ` Cyril Arnould
  3 siblings, 0 replies; 9+ messages in thread
From: Cyril Arnould @ 2023-07-06 19:28 UTC (permalink / raw)
  To: eliz@gnu.org; +Cc: acorallo@gnu.org, András Svraka, 63752@debbugs.gnu.org


[-- Attachment #1.1: Type: text/plain, Size: 4255 bytes --]

> It's too bad the problem goes away under GDB, because we don't even
> know whether the crash is due to some fatal error in the Emacs code,
> or due to those extra-checking routines inserted into the code by the
> FORTIFY option.  IOW, it could be that the problem happens because the
> FORTIFY routines don't understand something Emacs does and consider it
> a bug worthy of aborting the program.

Oh, I think there might be some confusion with #63365, sorry if I
haven't been precise enough. This fortify bug does *not* go away under
GDB (unless I'm misunderstanding). I get the following output when
running the binary compiled with -D_FORTIFY_SOURCE:

$ gdb -q --args ./src/emacs -Q --eval '(async-shell-command "dir")'
Reading symbols from ./src/emacs...
(gdb) run
Starting program: I:\Git\emacs\src\emacs.exe -Q --eval "(async-shell-command \"dir\")"
[New Thread 15740.0x22e0]
[New Thread 15740.0x2b50]
[New Thread 15740.0x10cc]
[New Thread 15740.0x4f4]
[New Thread 15740.0x3eac]
[New Thread 15740.0x3c80]

The output then continues once I close the window:

[Thread 15740.0x4f4 exited with code 0]
[New Thread 15740.0x3b00]
[New Thread 15740.0x9ac]
[Thread 15740.0x3c80 exited with code 0]
[Thread 15740.0x10cc exited with code 0]
[Thread 15740.0x2b50 exited with code 0]
[Thread 15740.0x22e0 exited with code 0]
[Thread 15740.0x3b00 exited with code 0]
[Thread 15740.0x3eac exited with code 0]
[Thread 15740.0x9ac exited with code 0]
[Inferior 1 (process 15740) exited normally]
(gdb) quit

When running the binary where sysdep.o is compiled without
-D_FORTIFY_SOURCE, I get the following instead:

$ gdb -q --args ./src/emacs -Q --eval '(async-shell-command "dir")'
Reading symbols from ./src/emacs...
(gdb) run
Starting program: I:\Git\emacs\src\emacs.exe -Q --eval "(async-shell-command \"dir\")"
[New Thread 4364.0x36d4]
[New Thread 4364.0x3f38]
[New Thread 4364.0x3eec]
[New Thread 4364.0x924]
[New Thread 4364.0x38bc]
[New Thread 4364.0x2f8c]
[Thread 4364.0x2f8c exited with code 0]

And once I close the window:

[Thread 4364.0x924 exited with code 0]
[Thread 4364.0x36d4 exited with code 0]
[Thread 4364.0x3eec exited with code 0]
[Thread 4364.0x3f38 exited with code 0]
[Thread 4364.0x38bc exited with code 0]
[Inferior 1 (process 4364) exited normally]
(gdb) quit

The big difference being that in the latter case, the last thread exits
immediately (which I assume is the "dir" command), while with
-D_FORTIFY_SOURCE the thread only exits once I close the window. I can
perform further tests with GDB of course.

> Another interesting question is why the problems happen only in Emacs
> built with native compilation.  Do we use emacs_read or its callers in
> some special ways when they are called from comp.c or comp.el?

Again, there's some confusion with #63365. AFAICT, the fortify bug
occurs regardless of whether Emacs is built --with-native-compilation or
not. The third emacs-28.2 release on MSYS2 had the fortify bug and was
compiled --with-native-compilation. So far in *this* bug report I have
been using the default ./configure options, so native compilation should
be off in my objdumps.

In fact, I think the two bugs are not related at all. #63365 occurs
regardless of whether Emacs is built with or without -D_FORTIFY_SOURCE;
see the fourth and fifth release of Emacs on MSYS2.

Now, to make sure the two are not related, I've now repeated the process
with the following flags:

CFLAGS='-g3 -O2 -gdwarf-2 -Wp,-D_FORTIFY_SOURCE=2 -fno-optimize-sibling-calls'

The fortify bug was still there, so it doesn't seem like
-foptimize-sibling-calls is the root cause for #63752. The objdump of
sysdep1.o is slightly different from the one compiled with
-foptimize-sibling-calls, I've attached it in case someone wants to take
a look but I don't think it's relevant.

> Maybe we should ask the MinGW64 folks who implemented the FORTIFY
> support for MinGW64 GCC to join this discussion and help us figure out
> which part(s) of this puzzle are relevant and why?  Cyril, can you
> reach out to them and ask them help us out here?

Sure, I've opened a support request:

https://sourceforge.net/p/mingw-w64/support-requests/193/

[-- Attachment #1.2: Type: text/html, Size: 8367 bytes --]

[-- Attachment #2: sysdep1-fortify-nosiblings.txt --]
[-- Type: text/plain, Size: 4368 bytes --]


src/sysdep1.o:     file format pe-x86-64


Disassembly of section .text:

0000000000000000 <emacs_intr_read>:
   if INTERRUPTIBLE, and then retry the read unless quitting.
   Return the number of bytes read, which might be less than NBYTE.
   On error, set errno to a value other than EINTR, and return -1.  */
static ptrdiff_t
emacs_intr_read (int fd, void *buf, ptrdiff_t nbyte, bool interruptible)
{
   0:	41 56                	push   %r14
   2:	41 55                	push   %r13
   4:	41 54                	push   %r12
   6:	55                   	push   %rbp
   7:	57                   	push   %rdi
   8:	56                   	push   %rsi
   9:	53                   	push   %rbx
   a:	48 83 ec 20          	sub    $0x20,%rsp
   e:	4c 8b 2d 00 00 00 00 	mov    0x0(%rip),%r13        # 15 <emacs_intr_read+0x15>
    {
      if (interruptible)
	maybe_quit ();
      result = read (fd, buf, nbyte);
    }
  while (result < 0 && errno == EINTR);
  15:	4c 8b 35 00 00 00 00 	mov    0x0(%rip),%r14        # 1c <emacs_intr_read+0x1c>
{
  1c:	41 89 cc             	mov    %ecx,%r12d
  1f:	48 89 d5             	mov    %rdx,%rbp
  22:	44 89 cf             	mov    %r9d,%edi
      result = read (fd, buf, nbyte);
  25:	44 89 c6             	mov    %r8d,%esi
  28:	eb 22                	jmp    4c <emacs_intr_read+0x4c>
  2a:	66 0f 1f 44 00 00    	nopw   0x0(%rax,%rax,1)

__mingw_bos_extern_ovr
int _read(int __fh, void * __dst, unsigned int __n)
{
  __mingw_bos_ptr_chk_warn(__dst, __n, 0);
  return __mingw_call__read(__fh, __dst, __n);
  30:	41 89 f0             	mov    %esi,%r8d
  33:	48 89 ea             	mov    %rbp,%rdx
  36:	44 89 e1             	mov    %r12d,%ecx
  39:	41 ff d5             	call   *%r13
  3c:	48 63 d8             	movslq %eax,%rbx
  while (result < 0 && errno == EINTR);
  3f:	48 85 db             	test   %rbx,%rbx
  42:	79 1c                	jns    60 <emacs_intr_read+0x60>
  44:	41 ff d6             	call   *%r14
  47:	83 38 04             	cmpl   $0x4,(%rax)
  4a:	75 14                	jne    60 <emacs_intr_read+0x60>
      if (interruptible)
  4c:	40 84 ff             	test   %dil,%dil
  4f:	74 df                	je     30 <emacs_intr_read+0x30>
	maybe_quit ();
  51:	e8 00 00 00 00       	call   56 <emacs_intr_read+0x56>
  56:	eb d8                	jmp    30 <emacs_intr_read+0x30>
  58:	0f 1f 84 00 00 00 00 	nopl   0x0(%rax,%rax,1)
  5f:	00 

  return result;
}
  60:	48 89 d8             	mov    %rbx,%rax
  63:	48 83 c4 20          	add    $0x20,%rsp
  67:	5b                   	pop    %rbx
  68:	5e                   	pop    %rsi
  69:	5f                   	pop    %rdi
  6a:	5d                   	pop    %rbp
  6b:	41 5c                	pop    %r12
  6d:	41 5d                	pop    %r13
  6f:	41 5e                	pop    %r14
  71:	c3                   	ret
  72:	66 66 2e 0f 1f 84 00 	data16 cs nopw 0x0(%rax,%rax,1)
  79:	00 00 00 00 
  7d:	0f 1f 00             	nopl   (%rax)

0000000000000080 <emacs_read>:

ptrdiff_t
emacs_read (int fd, void *buf, ptrdiff_t nbyte)
{
  80:	48 83 ec 28          	sub    $0x28,%rsp
  return emacs_intr_read (fd, buf, nbyte, false);
  84:	45 31 c9             	xor    %r9d,%r9d
  87:	e8 74 ff ff ff       	call   0 <emacs_intr_read>
}
  8c:	48 83 c4 28          	add    $0x28,%rsp
  90:	c3                   	ret
  91:	66 66 2e 0f 1f 84 00 	data16 cs nopw 0x0(%rax,%rax,1)
  98:	00 00 00 00 
  9c:	0f 1f 40 00          	nopl   0x0(%rax)

00000000000000a0 <emacs_read_quit>:

/* Like emacs_read, but also process quits and pending signals.  */
ptrdiff_t
emacs_read_quit (int fd, void *buf, ptrdiff_t nbyte)
{
  a0:	48 83 ec 28          	sub    $0x28,%rsp
  return emacs_intr_read (fd, buf, nbyte, true);
  a4:	41 b9 01 00 00 00    	mov    $0x1,%r9d
  aa:	e8 51 ff ff ff       	call   0 <emacs_intr_read>
}
  af:	48 83 c4 28          	add    $0x28,%rsp
  b3:	c3                   	ret
  b4:	90                   	nop
  b5:	90                   	nop
  b6:	90                   	nop
  b7:	90                   	nop
  b8:	90                   	nop
  b9:	90                   	nop
  ba:	90                   	nop
  bb:	90                   	nop
  bc:	90                   	nop
  bd:	90                   	nop
  be:	90                   	nop
  bf:	90                   	nop

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2023-07-06 19:28 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-27 12:57 bug#63752: 28.2; GCC 13.1 breaks Emacs subprocess handling when built with -D_FORTIFY_SOURCE Cyril Arnould
2023-05-27 13:42 ` Eli Zaretskii
2023-05-27 14:32   ` bug#63752: AW: " Cyril Arnould
2023-06-01  7:31     ` András Svraka
2023-06-30 22:41 ` Cyril Arnould
2023-07-01  6:40   ` Eli Zaretskii
2023-07-05 20:23 ` Cyril Arnould
2023-07-06  5:28   ` Eli Zaretskii
2023-07-06 19:28 ` Cyril Arnould

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).