unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
* bug#47596: File descriptor error when exiting emacs on android 11
@ 2021-04-04 19:20 Henrik Grimler
  2021-04-04 19:31 ` Eli Zaretskii
  0 siblings, 1 reply; 19+ messages in thread
From: Henrik Grimler @ 2021-04-04 19:20 UTC (permalink / raw)
  To: 47596

Hi,

Android 10 introduced a new "file descriptor sanitizer" (fdsan, see [1]
for docs) which detects issues when opening and closing files across
multiple threads. On android 10 warnings were given, while on android
11 programs are killed instantly if issues are detected. 

Starting and then exiting emacs, or simply just running for example
`emacs --version`, gives an error. `gdb --args emacs --version` gives
this backtrace:

```
Starting program: /data/data/com.termux/files/usr/bin/emacs --version
GNU Emacs 27.2
Copyright (C) 2021 Free Software Foundation, Inc.
GNU Emacs comes with ABSOLUTELY NO WARRANTY.
You may redistribute copies of GNU Emacs
under the terms of the GNU General Public License.
For more information about these matters, see the file named COPYING.
fdsan: attempted to close file descriptor 2, expected to be unowned,
actually owned by FILE* 0xb6c8800c

Program received signal SIGABRT, Aborted.
0xb63789e8 in fdsan_error(char const*, ...) ()
   from /apex/com.android.runtime/lib/bionic/libc.so
(gdb) bt full
#0  0xb63789e8 in fdsan_error(char const*, ...) ()
   from /apex/com.android.runtime/lib/bionic/libc.so
No symbol table info available.
#1  0xb63786fe in android_fdsan_close_with_tag ()
   from /apex/com.android.runtime/lib/bionic/libc.so
No symbol table info available.
#2  0xb63b83c0 in __sclose () from
/apex/com.android.runtime/lib/bionic/libc.so
No symbol table info available.
#3  0xb63b8f24 in __FILE_close(__sFILE*) ()
   from /apex/com.android.runtime/lib/bionic/libc.so
No symbol table info available.
#4  0x7f7e6a76 in close_stream (stream=0xb63cde44 <__sF+168>)
    at /home/builder/.termux-build/emacs/src/lib/close-stream.c:61
        some_pending = false
        prev_fail = false
        fclose_fail = 243
#5  0x7f68a872 in close_output_streams ()
    at /home/builder/.termux-build/emacs/src/src/sysdep.c:2840
        err = false
#6  0xb63c10d6 in __cxa_finalize ()
   from /apex/com.android.runtime/lib/bionic/libc.so
No symbol table info available.
#7  0xb63bc7b8 in exit () from
/apex/com.android.runtime/lib/bionic/libc.so
No symbol table info available.
#8  0x7f655d92 in main (argc=2, argv=0xbefff504)
    at /home/builder/.termux-build/emacs/src/src/emacs.c:1132
        version = 0xb5e68133 "27.2"
        copyright = 0xb5e68138 "Copyright (C) 2021 Free Software
Foundation, Inc."
        stack_bottom_variable = 0x7f655a61 <main>
        do_initial_setlocale = false
        no_loadup = false
        junk = 0x0
        dname_arg = 0x0
        ch_to_dir = 0x0
        original_pwd = 0x0
        dump_mode = 0x0
        skip_args = 1
        temacs = 0x0
        attempt_load_pdump = true
        sockfd = -1225460908
        module_assertions = 244
```

The emacs here was a debug build from the 27.2 tag, configured with:


 'configure --disable-dependency-tracking
 --prefix=/data/data/com.termux/files/usr
 --libdir=/data/data/com.termux/files/usr/lib
 --sbindir=/data/data/com.termux/files/usr/bin --disable-rpath
 --disable-rpath-hack --host=arm-linux-androideabi --disable-autodepend
 --with-gif=no --with-gnutls --with-jpeg=no --without-gconf
 --without-gsettings --without-lcms2 --without-x --with-png=no
 --with-tiff=no --with-xml2 --with-xpm=no --without-dbus
 --without-selinux --with-modules --with-pdumper=yes --with-
dumping=none
 --enable-checking=yes,glyphs --enable-check-lisp-object-type
 emacs_cv_sanitize_address=yes emacs_cv_prog_cc_no_pie=no
 ac_cv_lib_elf_elf_begin=no gl_cv_func_dup2_works=no
 ac_cv_func_setrlimit=no emacs_cv_func__setjmp=no
 emacs_cv_func_sigsetjmp=no --disable-nls --enable-shared
 --enable-static --libexecdir=/data/data/com.termux/files/usr/libexec
 'CFLAGS= -march=armv7-a -mfpu=neon -mfloat-abi=softfp -mthumb
 -fstack-protector-strong -g3 -O0 -Wall -gdwarf-4' 'CPPFLAGS=
 -D_FORTIFY_SOURCE=2 -D__USE_FORTIFY_LEVEL=2
 -I/data/data/com.termux/files/usr/include'
 'LDFLAGS=-L/data/data/com.termux/files/usr/lib
 -Wl,-rpath=/data/data/com.termux/files/usr/lib -march=armv7-a -fopenmp
 -static-openmp -Wl,--enable-new-dtags -Wl,--as-needed
 -Wl,-z,relro,-z,now''

Debugging these issues are very tedious in my experience so far
(probably easier for actual android apps). I will try to boil down the
relevant emacs code into a smaller program that still reproduces the
error.

Based on the backtrace it seem to be how stderr is opened (and closed)
that is problematic somehow.

Please let me know if you have any insights, or if I can provide
additional useful information.

Best regards,
Henrik Grimler

[1]
https://android.googlesource.com/platform/bionic/+/master/docs/fdsan.md






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

* bug#47596: File descriptor error when exiting emacs on android 11
  2021-04-04 19:20 bug#47596: File descriptor error when exiting emacs on android 11 Henrik Grimler
@ 2021-04-04 19:31 ` Eli Zaretskii
  2021-04-04 20:11   ` Henrik Grimler
  0 siblings, 1 reply; 19+ messages in thread
From: Eli Zaretskii @ 2021-04-04 19:31 UTC (permalink / raw)
  To: Henrik Grimler; +Cc: 47596

> From: Henrik Grimler <henrik@grimler.se>
> Date: Sun, 04 Apr 2021 21:20:35 +0200
> 
> Debugging these issues are very tedious in my experience so far
> (probably easier for actual android apps). I will try to boil down the
> relevant emacs code into a smaller program that still reproduces the
> error.

Yes, please.

> Based on the backtrace it seem to be how stderr is opened (and closed)
> that is problematic somehow.

Any idea what was "the other thread" involved in this situation?
Emacs is generally a single-threaded program.





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

* bug#47596: File descriptor error when exiting emacs on android 11
  2021-04-04 19:31 ` Eli Zaretskii
@ 2021-04-04 20:11   ` Henrik Grimler
  2021-04-05  8:14     ` Henrik Grimler
  0 siblings, 1 reply; 19+ messages in thread
From: Henrik Grimler @ 2021-04-04 20:11 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 47596


Hi,

> > Based on the backtrace it seem to be how stderr is opened (and
> > closed)
> > that is problematic somehow.
> 
> Any idea what was "the other thread" involved in this situation?
> Emacs is generally a single-threaded program.

I do not know unfortunately. I am not entirely certain the sanitizer is
trustworthy yet (but since it is active per default now I suppose it
should have been tested extensively). 

Tests so far show that optimisation level, and compiler, can influence
if these error occur or not, but for emacs it happens with -O0 as well
with clang-11. I opened an issue on the android issue tracker about the
fd sanitizer after investigating a fdsan error for texlive, since I do
not understand what the issue in the minimal example there is. We will
see what they say about it:
https://issuetracker.google.com/issues/184380442

Best regards,
Henrik Grimler






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

* bug#47596: File descriptor error when exiting emacs on android 11
  2021-04-04 20:11   ` Henrik Grimler
@ 2021-04-05  8:14     ` Henrik Grimler
  2021-04-05  8:59       ` Henrik Grimler
  2021-04-05 12:50       ` Eli Zaretskii
  0 siblings, 2 replies; 19+ messages in thread
From: Henrik Grimler @ 2021-04-05  8:14 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 47596

Hi,

Compiling this, on any optimisation level, is enough to trigger the
error:

```
#include <stdio.h>
int main()
{
  fdopen (2, "w");
  fclose (stderr);
}
```

In emacs fdopen is run in init_standard_fds, where we have

```
[...]

  force_open (STDERR_FILENO, O_RDONLY);

  /* Set buferr if possible on platforms defining _PC_PIPE_BUF, as
     they support the notion of atomic writes to pipes.  */
  #ifdef _PC_PIPE_BUF
    buferr = fdopen (STDERR_FILENO, "w");
    if (buferr)
      setvbuf (buferr, NULL, _IOLBF, 0);
  #endif
}
```

so I suppose there is either some very fundamental issue with 
`fdopen (STDERR_FILENO, "w")` here, or the file descriptor sanitizer on
android is broken.

If I avoid that part of init_standard_fds, i.e. change the ifdef to
something like `#if defined(_PC_PIPE_BUF) && !defined(__ANDROID__)`, I
get an emacs that seem to fully work on android.

Best regards,
Henrik Grimler






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

* bug#47596: File descriptor error when exiting emacs on android 11
  2021-04-05  8:14     ` Henrik Grimler
@ 2021-04-05  8:59       ` Henrik Grimler
  2021-04-05  9:48         ` Henrik Grimler
  2021-04-05 12:52         ` Eli Zaretskii
  2021-04-05 12:50       ` Eli Zaretskii
  1 sibling, 2 replies; 19+ messages in thread
From: Henrik Grimler @ 2021-04-05  8:59 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 47596

Hi again,


> Compiling this, on any optimisation level, is enough to trigger the
> error:
> 
> ```
> #include <stdio.h>
> int main()
> {
>   fdopen (2, "w");
>   fclose (stderr);
> }
> ```

Changing to this:

```

#include <stdio.h>
int main()
{
  FILE *err = fdopen (2, "w");
  fclose (err);
}
```

makes it work. So I suppose the sanitizer does not like that stderr is
closed with `fclose (stderr)` instead of by using the fd obtained from
fdopen (which was thrown away in my minimal example). 

Still not sure if this is actually problematic, but at least now I
understand how the sanitizer "thinks".

Best regards,
Henrik Grimler






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

* bug#47596: File descriptor error when exiting emacs on android 11
  2021-04-05  8:59       ` Henrik Grimler
@ 2021-04-05  9:48         ` Henrik Grimler
  2021-04-05 12:57           ` Eli Zaretskii
  2021-04-05 12:52         ` Eli Zaretskii
  1 sibling, 1 reply; 19+ messages in thread
From: Henrik Grimler @ 2021-04-05  9:48 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 47596

Hi,

A better way to fix the error is to use buferr instead of stderr when
calling `close_streams` in `close_output_streams`. I cannot see any
obvious problems with this, so I propose this patch:

```

--- ./src/sysdep.c.orig	2021-04-05 09:06:33.847835653 +0000
+++ ./src/sysdep.c	2021-04-05 09:05:47.957856162 +0000
@@ -2837,8 +2837,8 @@
      sanitizer might report to stderr after this function is invoked.
*/
   bool err = buferr && (fflush (buferr) != 0 || ferror (buferr));
   if (err | (ADDRESS_SANITIZER
-	     ? fflush (stderr) != 0 || ferror (stderr)
-	     : close_stream (stderr) != 0))
+	     ? fflush (buferr) != 0 || ferror (buferr)
+	     : close_stream (buferr) != 0))
     _exit (EXIT_FAILURE);
 }
 ^L
```

With it I can start and exit emacs on android without the fdsan error. 

Best regards,
Henrik Grimler






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

* bug#47596: File descriptor error when exiting emacs on android 11
  2021-04-05  8:14     ` Henrik Grimler
  2021-04-05  8:59       ` Henrik Grimler
@ 2021-04-05 12:50       ` Eli Zaretskii
  2021-04-05 17:29         ` Henrik Grimler
  1 sibling, 1 reply; 19+ messages in thread
From: Eli Zaretskii @ 2021-04-05 12:50 UTC (permalink / raw)
  To: Henrik Grimler; +Cc: 47596

> From: Henrik Grimler <henrik@grimler.se>
> Cc: 47596@debbugs.gnu.org
> Date: Mon, 05 Apr 2021 10:14:10 +0200
> 
> Compiling this, on any optimisation level, is enough to trigger the
> error:
> 
> ```
> #include <stdio.h>
> int main()
> {
>   fdopen (2, "w");
>   fclose (stderr);
> }
> ```

Any idea why?  It makes no sense to me, and the above is an entirely
valid program, AFAICT.

> In emacs fdopen is run in init_standard_fds, where we have
> 
> ```
> [...]
> 
>   force_open (STDERR_FILENO, O_RDONLY);
> 
>   /* Set buferr if possible on platforms defining _PC_PIPE_BUF, as
>      they support the notion of atomic writes to pipes.  */
>   #ifdef _PC_PIPE_BUF
>     buferr = fdopen (STDERR_FILENO, "w");
>     if (buferr)
>       setvbuf (buferr, NULL, _IOLBF, 0);
>   #endif
> }
> ```

This just creates a copy of stderr that has special buffering.  Again,
entirely valid for a C program to do that.

> so I suppose there is either some very fundamental issue with 
> `fdopen (STDERR_FILENO, "w")` here, or the file descriptor sanitizer on
> android is broken.

Likely the latter, so I'm reluctant to make any changes in Emacs until
the issue you opened against Android gets some response that makes
sense.

> If I avoid that part of init_standard_fds, i.e. change the ifdef to
> something like `#if defined(_PC_PIPE_BUF) && !defined(__ANDROID__)`, I
> get an emacs that seem to fully work on android.

Feel free to make such changes in your copy, but I'd prefer not to
change Emacs until the root cause behind these weird problems becomes
clear.





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

* bug#47596: File descriptor error when exiting emacs on android 11
  2021-04-05  8:59       ` Henrik Grimler
  2021-04-05  9:48         ` Henrik Grimler
@ 2021-04-05 12:52         ` Eli Zaretskii
  1 sibling, 0 replies; 19+ messages in thread
From: Eli Zaretskii @ 2021-04-05 12:52 UTC (permalink / raw)
  To: Henrik Grimler; +Cc: 47596

> From: Henrik Grimler <henrik@grimler.se>
> Cc: 47596@debbugs.gnu.org
> Date: Mon, 05 Apr 2021 10:59:55 +0200
> 
> > #include <stdio.h>
> > int main()
> > {
> >   fdopen (2, "w");
> >   fclose (stderr);
> > }
> > ```
> 
> Changing to this:
> 
> ```
> 
> #include <stdio.h>
> int main()
> {
>   FILE *err = fdopen (2, "w");
>   fclose (err);
> }
> ```
> 
> makes it work.

Which again makes no sense, because the program that works is a no-op:
it creates a copy of stderr and immediately closes it.

> So I suppose the sanitizer does not like that stderr is
> closed with `fclose (stderr)` instead of by using the fd obtained from
> fdopen (which was thrown away in my minimal example). 
> 
> Still not sure if this is actually problematic, but at least now I
> understand how the sanitizer "thinks".

If that's what it thinks, it's a clear bug in the sanitizer, IMO.





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

* bug#47596: File descriptor error when exiting emacs on android 11
  2021-04-05  9:48         ` Henrik Grimler
@ 2021-04-05 12:57           ` Eli Zaretskii
  2021-04-05 13:38             ` Henrik Grimler
  0 siblings, 1 reply; 19+ messages in thread
From: Eli Zaretskii @ 2021-04-05 12:57 UTC (permalink / raw)
  To: Henrik Grimler; +Cc: 47596

> From: Henrik Grimler <henrik@grimler.se>
> Cc: 47596@debbugs.gnu.org
> Date: Mon, 05 Apr 2021 11:48:50 +0200
> 
> A better way to fix the error is to use buferr instead of stderr when
> calling `close_streams` in `close_output_streams`. I cannot see any
> obvious problems with this, so I propose this patch:

You are operating on the wrong stream, so I do see a problem with this
patch.  The purpose of this fragment is to close stderr, not the
buffered copy of stderr.

> --- ./src/sysdep.c.orig	2021-04-05 09:06:33.847835653 +0000
> +++ ./src/sysdep.c	2021-04-05 09:05:47.957856162 +0000
> @@ -2837,8 +2837,8 @@
>       sanitizer might report to stderr after this function is invoked.
> */
>    bool err = buferr && (fflush (buferr) != 0 || ferror (buferr));
>    if (err | (ADDRESS_SANITIZER
> -	     ? fflush (stderr) != 0 || ferror (stderr)
> -	     : close_stream (stderr) != 0))
> +	     ? fflush (buferr) != 0 || ferror (buferr)
> +	     : close_stream (buferr) != 0))
>      _exit (EXIT_FAILURE);
>  }
>  ^L
> ```
> 
> With it I can start and exit emacs on android without the fdsan error. 

Is ADDRESS_SANITIZER defined in your build?

In any case, I do want to wait for the Android developers to respond
to the issue in their tracker.

Thanks.





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

* bug#47596: File descriptor error when exiting emacs on android 11
  2021-04-05 12:57           ` Eli Zaretskii
@ 2021-04-05 13:38             ` Henrik Grimler
  0 siblings, 0 replies; 19+ messages in thread
From: Henrik Grimler @ 2021-04-05 13:38 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 47596


Hi,

> You are operating on the wrong stream, so I do see a problem with this
> patch.  The purpose of this fragment is to close stderr, not the
> buffered copy of stderr.

> 
Thanks for the feedback!

> Is ADDRESS_SANITIZER defined in your build?

Yes, it is configured with emacs_cv_sanitize_address=yes. Setting this
option has been done for android since emacs 25, and supposedly fixed
some malloc issue back then [1]. I tried building without setting
emacs_cv_sanitize_address=yes just now (configure then sets =no), but
it does not seem to influence this fdsan error.

> In any case, I do want to wait for the Android developers to respond
> to the issue in their tracker.

I will update the issue and add the minimal non-working example
obtained here as well.

Best regards,
Henrik Grimler

[1]
https://github.com/termux/termux-packages/commit/8ce98d7fe9130cd151cc2ec34b3b1e7ecb0aeace#diff-8fd6b84ed67057870fa4f209d2309b53bf3c097b732cb3b8e639f58a37d138c1R7






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

* bug#47596: File descriptor error when exiting emacs on android 11
  2021-04-05 12:50       ` Eli Zaretskii
@ 2021-04-05 17:29         ` Henrik Grimler
  2021-05-06 10:45           ` Lars Ingebrigtsen
  0 siblings, 1 reply; 19+ messages in thread
From: Henrik Grimler @ 2021-04-05 17:29 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 47596

Hi,

I received feedback on the android issue tracker 
https://issuetracker.google.com/issues/184380442 
that the snippet:

> > #include <stdio.h>
> > int main()
> > {
> >   fdopen (2, "w");
> >   fclose (stderr);
> > }

is problematic, because:

> yes. you have two FILE*s that both think they own file descriptor 2.
> depending on what you're actually trying to do, you probably meant to
> use freopen(3) instead?
https://man7.org/linux/man-pages/man3/fopen.3.html

Does freopen make sense for buferr?

> > In emacs fdopen is run in init_standard_fds, where we have
> > 
> >   force_open (STDERR_FILENO, O_RDONLY);
> > 
> >   /* Set buferr if possible on platforms defining _PC_PIPE_BUF, as
> >      they support the notion of atomic writes to pipes.  */
> >   #ifdef _PC_PIPE_BUF
> >     buferr = fdopen (STDERR_FILENO, "w");
> >     if (buferr)
> >       setvbuf (buferr, NULL, _IOLBF, 0);
> >   #endif
> > }
> 
> This just creates a copy of stderr that has special buffering. 
> Again,
> entirely valid for a C program to do that.

I probably should have included a bit more context in the android bug
report to better show what the code does. 

Best regards,
Henrik Grimler







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

* bug#47596: File descriptor error when exiting emacs on android 11
  2021-04-05 17:29         ` Henrik Grimler
@ 2021-05-06 10:45           ` Lars Ingebrigtsen
  2021-05-10  7:23             ` Henrik Grimler
  0 siblings, 1 reply; 19+ messages in thread
From: Lars Ingebrigtsen @ 2021-05-06 10:45 UTC (permalink / raw)
  To: Henrik Grimler; +Cc: 47596

Henrik Grimler <henrik@grimler.se> writes:

>> > In emacs fdopen is run in init_standard_fds, where we have
>> > 
>> >   force_open (STDERR_FILENO, O_RDONLY);
>> > 
>> >   /* Set buferr if possible on platforms defining _PC_PIPE_BUF, as
>> >      they support the notion of atomic writes to pipes.  */
>> >   #ifdef _PC_PIPE_BUF
>> >     buferr = fdopen (STDERR_FILENO, "w");
>> >     if (buferr)
>> >       setvbuf (buferr, NULL, _IOLBF, 0);
>> >   #endif
>> > }
>> 
>> This just creates a copy of stderr that has special buffering. 
>> Again,
>> entirely valid for a C program to do that.
>
> I probably should have included a bit more context in the android bug
> report to better show what the code does. 

Did you try to include this context in the Android bug report and see
what they say then?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no





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

* bug#47596: File descriptor error when exiting emacs on android 11
  2021-05-06 10:45           ` Lars Ingebrigtsen
@ 2021-05-10  7:23             ` Henrik Grimler
  2021-05-11 17:06               ` Henrik Grimler
  0 siblings, 1 reply; 19+ messages in thread
From: Henrik Grimler @ 2021-05-10  7:23 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: 47596

Hi Lars,

On Thu, 2021-05-06 at 12:45 +0200, Lars Ingebrigtsen wrote:
> > I probably should have included a bit more context in the android bug
> > report to better show what the code does. 
> 
> Did you try to include this context in the Android bug report and see
> what they say then?
> 

I added another comment now with a slightly more verbose example: 

```
#include <stdlib.h>
#include <unistd.h>

/* If FD is not already open, arrange for it to be open with FLAGS.  */
static void
force_open (int fd, int flags)
{
  if (dup2 (fd, fd) < 0)
    {
      int n = open ("/dev/null", flags);
      if (n < 0 || (fd != n && (dup2 (n, fd) < 0 || close (n) == 0 !=
0)))
	exit (EXIT_FAILURE);
    }
}

/* A stream that is like stderr, except line buffered.  It is NULL
   during startup, or if line buffering is not in use.  */
static FILE *buferr;

int
main()
{
  /* Make sure stderr are open to something, so that the file
   descriptor is not hijacked by later system calls.  */
  force_open (STDERR_FILENO, O_RDONLY);

  /* Set buferr if possible on platforms defining _PC_PIPE_BUF, as
     they support the notion of atomic writes to pipes.  */
  #ifdef _PC_PIPE_BUF
    buferr = fdopen (STDERR_FILENO, "w");
    if (buferr)
      setvbuf (buferr, NULL, _IOLBF, 0);
  #endif

  int err = buferr && (fflush (buferr) != 0 || ferror (buferr));
  if (err | (fclose (stderr) != 0))
    return 1;
  return 0;
}
```

Hopefully they have time to provide some feedback.

Best regards,
Henrik Grimler






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

* bug#47596: File descriptor error when exiting emacs on android 11
  2021-05-10  7:23             ` Henrik Grimler
@ 2021-05-11 17:06               ` Henrik Grimler
  2021-05-12 13:52                 ` Lars Ingebrigtsen
  0 siblings, 1 reply; 19+ messages in thread
From: Henrik Grimler @ 2021-05-11 17:06 UTC (permalink / raw)
  To: Lars Ingebrigtsen, eliz; +Cc: 47596

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

Hi again,

Enh at google suggested [1] that `setlinebuf (stderr)` could be an
alternative instead of opening a copy of stderr.  I tried out the
attached patch, which removes the buferr variable and uses setlinebuf
on stderr instead, and it seem to work (in the sense that emacs opens
and closes without errors) on archlinux as well as android. 

Could this work or do we need to have both buferr and stderr?

Best regards,
Henrik Grimler

[1] https://issuetracker.google.com/issues/184380442

[-- Attachment #2: line_buffered_stderr.patch --]
[-- Type: text/x-patch, Size: 2343 bytes --]

commit ef3beaf664347f7fce1f3475fe65de729320837e
Author: Henrik Grimler <henrik@grimler.se>
Date:   Tue May 11 09:36:52 2021 +0200

    Use setlinebuf to make stderr line buffered
    
    If atomic writes are supported.  Android >= 10 has a file descriptor
    sanitizer which errors if a stream is closed while there are still
    fd's associated with it. In emacs stderr is opened twice if line
    buffered streams are supported.  Rather than opening a copy we can
    make stderr line buffered with setlinebuf instead.

diff --git a/src/sysdep.c b/src/sysdep.c
index d940acc4e0..77e1a0be3c 100644
--- a/src/sysdep.c
+++ b/src/sysdep.c
@@ -232,10 +232,6 @@ force_open (int fd, int flags)
     }
 }
 
-/* A stream that is like stderr, except line buffered.  It is NULL
-   during startup, or if line buffering is not in use.  */
-static FILE *buferr;
-
 /* Make sure stdin, stdout, and stderr are open to something, so that
    their file descriptors are not hijacked by later system calls.  */
 void
@@ -249,12 +245,10 @@ init_standard_fds (void)
   force_open (STDOUT_FILENO, O_RDONLY);
   force_open (STDERR_FILENO, O_RDONLY);
 
-  /* Set buferr if possible on platforms defining _PC_PIPE_BUF, as
+  /* Make stderr line buffered on platforms defining _PC_PIPE_BUF, as
      they support the notion of atomic writes to pipes.  */
   #ifdef _PC_PIPE_BUF
-    buferr = fdopen (STDERR_FILENO, "w");
-    if (buferr)
-      setvbuf (buferr, NULL, _IOLBF, 0);
+    setlinebuf (stderr);
   #endif
 }
 
@@ -2650,11 +2644,11 @@ safe_strsignal (int code)
 static FILE *
 errstream (void)
 {
-  FILE *err = buferr;
-  if (!err)
-    return stderr;
+#ifdef _PC_PIPE_BUF
+  /* flush line buffered stderr */
   fflush_unlocked (stderr);
-  return err;
+#endif
+  return stderr;
 }
 
 /* These functions are like fputc, vfprintf, and fwrite,
@@ -2696,9 +2690,12 @@ close_output_streams (void)
       _exit (EXIT_FAILURE);
     }
 
+  bool err;
+#ifdef _PC_PIPE_BUF
+  err = (fflush (stderr) != 0 || ferror (stderr));
+#endif
   /* Do not close stderr if addresses are being sanitized, as the
      sanitizer might report to stderr after this function is invoked.  */
-  bool err = buferr && (fflush (buferr) != 0 || ferror (buferr));
   if (err | (ADDRESS_SANITIZER
 	     ? fflush (stderr) != 0 || ferror (stderr)
 	     : close_stream (stderr) != 0))

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

* bug#47596: File descriptor error when exiting emacs on android 11
  2021-05-11 17:06               ` Henrik Grimler
@ 2021-05-12 13:52                 ` Lars Ingebrigtsen
  2021-05-12 14:01                   ` Eli Zaretskii
  0 siblings, 1 reply; 19+ messages in thread
From: Lars Ingebrigtsen @ 2021-05-12 13:52 UTC (permalink / raw)
  To: Henrik Grimler; +Cc: 47596

Henrik Grimler <henrik@grimler.se> writes:

> Enh at google suggested [1] that `setlinebuf (stderr)` could be an
> alternative instead of opening a copy of stderr.  I tried out the
> attached patch, which removes the buferr variable and uses setlinebuf
> on stderr instead, and it seem to work (in the sense that emacs opens
> and closes without errors) on archlinux as well as android. 
>
> Could this work or do we need to have both buferr and stderr?

Makes sense to me, but I seem to vaguely recall there being a lot of
discussion about the buffered stderr when it was introduced...  but none
of the details.  Eli?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no





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

* bug#47596: File descriptor error when exiting emacs on android 11
  2021-05-12 13:52                 ` Lars Ingebrigtsen
@ 2021-05-12 14:01                   ` Eli Zaretskii
  2022-06-29 10:47                     ` Lars Ingebrigtsen
  0 siblings, 1 reply; 19+ messages in thread
From: Eli Zaretskii @ 2021-05-12 14:01 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: henrik, 47596

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: eliz@gnu.org,  47596@debbugs.gnu.org
> Date: Wed, 12 May 2021 15:52:52 +0200
> 
> Henrik Grimler <henrik@grimler.se> writes:
> 
> > Could this work or do we need to have both buferr and stderr?
> 
> Makes sense to me, but I seem to vaguely recall there being a lot of
> discussion about the buffered stderr when it was introduced...  but none
> of the details.  Eli?

We need them both, yes.  The second one was introduced for a reason,
so removing it would give us back the problems it solved.

setlinebuf is not portable enough, and line buffering isn't supported
on MS-Windows anyway.  Maybe we could make some archlinux specific
change, although that's ugly.

And I'm worried that we don't actually understand the nature of the
problem well enough: our code doesn't close both FILE streams, it
closes at most only one.  So this sounds to me like a false positive
of the sanitizer, not a real problem with our code...





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

* bug#47596: File descriptor error when exiting emacs on android 11
  2021-05-12 14:01                   ` Eli Zaretskii
@ 2022-06-29 10:47                     ` Lars Ingebrigtsen
  2022-06-29 13:25                       ` Henrik Grimler via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 1 reply; 19+ messages in thread
From: Lars Ingebrigtsen @ 2022-06-29 10:47 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: henrik, 47596

Eli Zaretskii <eliz@gnu.org> writes:

> And I'm worried that we don't actually understand the nature of the
> problem well enough: our code doesn't close both FILE streams, it
> closes at most only one.  So this sounds to me like a false positive
> of the sanitizer, not a real problem with our code...

Henrik, is this still a problem with current versions of Emacs/Android?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no





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

* bug#47596: File descriptor error when exiting emacs on android 11
  2022-06-29 10:47                     ` Lars Ingebrigtsen
@ 2022-06-29 13:25                       ` Henrik Grimler via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2022-06-30  9:07                         ` Lars Ingebrigtsen
  0 siblings, 1 reply; 19+ messages in thread
From: Henrik Grimler via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2022-06-29 13:25 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: Eli Zaretskii, 47596

Hi Lars,

On Wed, Jun 29, 2022 at 12:47:27PM +0200, Lars Ingebrigtsen wrote:
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > And I'm worried that we don't actually understand the nature of the
> > problem well enough: our code doesn't close both FILE streams, it
> > closes at most only one.  So this sounds to me like a false positive
> > of the sanitizer, not a real problem with our code...
> 
> Henrik, is this still a problem with current versions of Emacs/Android?

The sanitizer still complains about it (just checked a new emacs build
from master branch), but we have at least figured out a way to disable
the sanitizer [1], so IMO the bug can be closed.

For the record ~10 different packages out of around 2000 has triggered
this file descriptor sanitizer (that I've seen so far), and in most
cases it has been as unclear as for emacs what the issue is/if there
actually is an issue.

> -- 
> (domestic pets only, the antidote for overdose, milk.)
>    bloggy blog: http://lars.ingebrigtsen.no

[1] https://github.com/termux/termux-packages/blob/master/packages/emacs/disable-fdsan.patch

Best regards,
Henrik Grimler





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

* bug#47596: File descriptor error when exiting emacs on android 11
  2022-06-29 13:25                       ` Henrik Grimler via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2022-06-30  9:07                         ` Lars Ingebrigtsen
  0 siblings, 0 replies; 19+ messages in thread
From: Lars Ingebrigtsen @ 2022-06-30  9:07 UTC (permalink / raw)
  To: Henrik Grimler; +Cc: Eli Zaretskii, 47596

Henrik Grimler <henrik@grimler.se> writes:

> The sanitizer still complains about it (just checked a new emacs build
> from master branch), but we have at least figured out a way to disable
> the sanitizer [1], so IMO the bug can be closed.
>
> For the record ~10 different packages out of around 2000 has triggered
> this file descriptor sanitizer (that I've seen so far), and in most
> cases it has been as unclear as for emacs what the issue is/if there
> actually is an issue.

Ah, I see.  OK, I guess there's nothing to be done on the Emacs side
here, then, and I'm closing this bug report.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no





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

end of thread, other threads:[~2022-06-30  9:07 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-04 19:20 bug#47596: File descriptor error when exiting emacs on android 11 Henrik Grimler
2021-04-04 19:31 ` Eli Zaretskii
2021-04-04 20:11   ` Henrik Grimler
2021-04-05  8:14     ` Henrik Grimler
2021-04-05  8:59       ` Henrik Grimler
2021-04-05  9:48         ` Henrik Grimler
2021-04-05 12:57           ` Eli Zaretskii
2021-04-05 13:38             ` Henrik Grimler
2021-04-05 12:52         ` Eli Zaretskii
2021-04-05 12:50       ` Eli Zaretskii
2021-04-05 17:29         ` Henrik Grimler
2021-05-06 10:45           ` Lars Ingebrigtsen
2021-05-10  7:23             ` Henrik Grimler
2021-05-11 17:06               ` Henrik Grimler
2021-05-12 13:52                 ` Lars Ingebrigtsen
2021-05-12 14:01                   ` Eli Zaretskii
2022-06-29 10:47                     ` Lars Ingebrigtsen
2022-06-29 13:25                       ` Henrik Grimler via Bug reports for GNU Emacs, the Swiss army knife of text editors
2022-06-30  9:07                         ` Lars Ingebrigtsen

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).