[Regression/Android] Commit e7fb52b3 causes segfault on 32-bit platforms

[Regression/Android] Commit e7fb52b3 causes segfault on 32-bit platforms

[Tom Yan]

Hi all,

http://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=blobdiff;f=lib/stdio-impl.h;h=393ef0cf58d013ec18cfcbb2ee120c4d1df7e028;hp=7a0ac4d50b70c5ad3d3b63d7f3274ba5d6df43a3;hb=e7fb52b3a5ddffe1bba848e4966159868b6951f0;hpb=be18f9170e1989b601a08aada31438cca87cffdd

The definition of fp_ is causing segfault on arm/i686 and doesn't seem
necessary on aarch64/x86_64 as expected either (as of Android 8.0).

See more discussion here: https://github.com/termux/termux-packages/issues/2603

Regards,
Tom

Re: [Regression/Android] Commit e7fb52b3 causes segfault on 32-bit platforms

[Paul Eggert]

Tom Yan wrote:
> See more discussion here:https://github.com/termux/termux-packages/issues/2603

Thanks. That discussion talks about "coreutils 8.30-1"; what's that and where is 
its source code?

I see this patch:

https://github.com/termux/termux-packages/commit/86af1159d0d487368aab2494904e89b86f042fe2

but it's not for 8.30-1. Also, the patch looks like it won't work on some 
Android platforms.

Re: [Regression/Android] Commit e7fb52b3 causes segfault on 32-bit platforms

[Tom Yan]

On Wed, 4 Jul 2018 at 04:22, Paul Eggert <eggert@cs.ucla.edu> wrote:
>
> Thanks. That discussion talks about "coreutils 8.30-1"; what's that and where is
> its source code?

GNU coreutils 8.30. GNU coreutils 8.29 uses a pre-e7fb52b3 snapshot of
gnulib, while 8.30 uses a post-e7fb52b3 snapshot of it.

> I see this patch:
>
> https://github.com/termux/termux-packages/commit/86af1159d0d487368aab2494904e89b86f042fe2
>
> but it's not for 8.30-1. Also, the patch looks like it won't work on some
> Android platforms.

Since the newly introduced definition of fp_ in stdio-impl.h is found
to be causing segfaults on quite some (if not most/all) 32-bit Android
builds, we patch it (hence 8.30"-1") to avoid the definition (on all
platforms).

We don't know how exactly the definition is problematic, or whether it
is really necessary/unnecessary for any ancient/upcomimg Android
releases on any platform. So far there's no report of side-effects
from our workaround though.

Re: [Regression/Android] Commit e7fb52b3 causes segfault on 32-bit platforms

[Paul Eggert]

Tom Yan wrote:
> Since the newly introduced definition of fp_ in stdio-impl.h is found
> to be causing segfaults on quite some (if not most/all) 32-bit Android
> builds, we patch it (hence 8.30"-1") to avoid the definition (on all
> platforms).

With what patch, exactly?

Re: [Regression/Android] Commit e7fb52b3 causes segfault on 32-bit platforms

[Bruno Haible]

Tom Yan wrote:
> http://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=blobdiff;f=lib/stdio-impl.h;h=393ef0cf58d013ec18cfcbb2ee120c4d1df7e028;hp=7a0ac4d50b70c5ad3d3b63d7f3274ba5d6df43a3;hb=e7fb52b3a5ddffe1bba848e4966159868b6951f0;hpb=be18f9170e1989b601a08aada31438cca87cffdd
> 
> The definition of fp_ is causing segfault on arm/i686 and doesn't seem
> necessary on aarch64/x86_64 as expected either (as of Android 8.0).

1) What version of Bionic does this version of Android use?
   I need a tag for https://android.googlesource.com/platform/bionic.git/
   or at least an approximation.

2) What is the highest version of Android for which it does not crash?

Bruno

Re: [Regression/Android] Commit e7fb52b3 causes segfault on 32-bit platforms

[Tom Yan]

I don't think most (if not all) reporters can provide the exact
version of bionic they are on, though you are welcome to ask on the
github issue I linked. Quite a few reporters were with 32-bit Android
7.1.1 I think.

I myself am with 64-bit Android 8.0.0 (Not an AOSP build but a Sony
stock ROM). I can also reproduce the issue if I set up a 32-bit prefix
(which makes use of the 32-bit /system/lib).
On Wed, 4 Jul 2018 at 10:18, Bruno Haible <bruno@clisp.org> wrote:
>
> Tom Yan wrote:
> > http://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=blobdiff;f=lib/stdio-impl.h;h=393ef0cf58d013ec18cfcbb2ee120c4d1df7e028;hp=7a0ac4d50b70c5ad3d3b63d7f3274ba5d6df43a3;hb=e7fb52b3a5ddffe1bba848e4966159868b6951f0;hpb=be18f9170e1989b601a08aada31438cca87cffdd
> >
> > The definition of fp_ is causing segfault on arm/i686 and doesn't seem
> > necessary on aarch64/x86_64 as expected either (as of Android 8.0).
>
> 1) What version of Bionic does this version of Android use?
>    I need a tag for https://android.googlesource.com/platform/bionic.git/
>    or at least an approximation.
>
> 2) What is the highest version of Android for which it does not crash?
>
> Bruno
>

Re: [Regression/Android] Commit e7fb52b3 causes segfault on 32-bit platforms

[Paul Eggert]

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

bore wrote:
> What is causing this is
> int  _flags; \
> int  _file; \
> on 32bit it needs to be
> short _flags; \
> short _file; \

Thanks for the suggestion. Does the attached Gnulib patch fix the problem? I 
haven't installed it and can't easily test it.

[-- Attachment #2: 0001-stdioext-port-to-newer-32-bit-Android.patch --]
[-- Type: text/x-patch, Size: 2066 bytes --]

From 8f87a66104c1e270d3ada0eb2753ba835f7aa498 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sat, 22 Dec 2018 18:00:10 -0800
Subject: [PATCH] stdioext: port to newer 32-bit Android

Problem reported by Tom Yan in:
https://lists.gnu.org/archive/html/bug-gnulib/2018-07/msg00014.html
* lib/stdio-impl.h (_gl_FILE_flags_t) [__ANDROID__]: New macro.
(fp_) [__ANDROID__]: Use it.
---
 ChangeLog        | 8 ++++++++
 lib/stdio-impl.h | 9 +++++++--
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index d1f0d630e..3caba1491 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2018-12-22  Paul Eggert  <eggert@cs.ucla.edu>
+
+	stdioext: port to newer 32-bit Android
+	Problem reported by Tom Yan in:
+	https://lists.gnu.org/archive/html/bug-gnulib/2018-07/msg00014.html
+	* lib/stdio-impl.h (_gl_FILE_flags_t) [__ANDROID__]: New macro.
+	(fp_) [__ANDROID__]: Use it.
+
 2018-12-21  Bruno Haible  <bruno@clisp.org>
 
 	Assume Autoconf >= 2.63.
diff --git a/lib/stdio-impl.h b/lib/stdio-impl.h
index 393ef0cf5..e198f09ed 100644
--- a/lib/stdio-impl.h
+++ b/lib/stdio-impl.h
@@ -61,6 +61,11 @@
 #  define _r pub._r
 #  define _w pub._w
 # elif defined __ANDROID__ /* Android */
+#  ifdef __LP64__
+#   define _gl_flags_file_t int
+#  else
+#   define _gl_flags_file_t short
+#  endif
   /* Up to this commit from 2015-10-12
      <https://android.googlesource.com/platform/bionic.git/+/f0141dfab10a4b332769d52fa76631a64741297a>
      the innards of FILE were public, and fp_ub could be defined like for OpenBSD,
@@ -70,8 +75,8 @@
 #  define fp_ ((struct { unsigned char *_p; \
                          int _r; \
                          int _w; \
-                         int _flags; \
-                         int _file; \
+                         _gl_flags_file_t _flags; \
+                         _gl_flags_file_t _file; \
                          struct { unsigned char *_base; size_t _size; } _bf; \
                          int _lbfsize; \
                          void *_cookie; \
-- 
2.17.1

Re: [Regression/Android] Commit e7fb52b3 causes segfault on 32-bit platforms

[Bruno Haible]

Paul Eggert wrote on 2018-12-22:
> bore wrote:
> > What is causing this is
> > int  _flags; \
> > int  _file; \
> > on 32bit it needs to be
> > short _flags; \
> > short _file; \
> 
> Thanks for the suggestion. Does the attached Gnulib patch fix the problem? I 
> haven't installed it and can't easily test it.

The patch is evidently better than the previous code I had put in. Thanks.
I installed it.

Bruno