jemalloc-sys: hardening strerror_r function detection

[BusyJay]

This PR also removes stale arch conditions.

/cc @th7nder can you verify it still works on Nix?

[th7nder]

It does not. I pulled the branch and run it on a flake.
How about we do something like this:

 124   │ diff --git a/jemalloc-sys/build.rs b/jemalloc-sys/build.rs
 125   │ index b47ee36..923bf39 100644
 126   │ --- a/jemalloc-sys/build.rs
 127   │ +++ b/jemalloc-sys/build.rs
 128   │ @@ -146,19 +146,9 @@ fn main() {
 129   │          return;
 130   │      }
 131   │      let mut build = cc::Build::new();
 132   │ -    if env::var("JEMALLOC_SYS_RUN_JEMALLOC_TESTS").is_err() {
 133   │ -        // Disable -Wall unless in CI.
 134   │ -        build.warnings(false);
 135   │ -    }
 136   │ +    build.warnings(false);
 137   │      let compiler = build.get_compiler();
 138   │ -    let cflags = compiler
 139   │ -        .args()
 140   │ -        .iter()
 141   │ -        .map(|s| s.to_str().unwrap())
 142   │ -        .collect::<Vec<_>>()
 143   │ -        .join(" ");
 144   │      info!("CC={:?}", compiler.path());
 145   │ -    info!("CFLAGS={:?}", cflags);
 146   │  
 147   │      assert!(out_dir.exists(), "OUT_DIR does not exist");
 148   │      let jemalloc_repo_dir = PathBuf::from("jemalloc");
 149   │ @@ -193,9 +183,6 @@ fn main() {
 150   │      )
 151   │      .current_dir(&build_dir)
 152   │      .env("CC", compiler.path())
 153   │ -    .env("CFLAGS", cflags.clone())
 154   │ -    .env("LDFLAGS", cflags.clone())
 155   │ -    .env("CPPFLAGS", cflags)
 156   │      .arg(format!("--with-version={je_version}"))
 157   │      .arg("--disable-cxx")
 158   │      .arg("--enable-doc=no")

So basically specify build.warnings(false) but don't pass CFLAGS/LDFLAGS (which are causing issues).
I tested it locally and it works.

[th7nder]

WIth those suggestions, we disable warnings but don't pass unnecessary stuff in to the compiler during the phases.

[BusyJay]

According to your previous analysis in #116, the failure is caused by -Wall flag, and the flag is removed by build.warnings(false). So passing the cflags to build script should be OK.

The reason why I keep pass cflags is because cc already does tweak for various platforms to works with Rust, not passing it down may cause unexpected behavior.

[th7nder]

The failure is not caused by passing -Wall flags, but a bit more indirect.

When you pass default compiler flags to CFLAGS/LDFLAGS, so -O0 -Wall. They end up being in the CONFIGURE_CFLAGS.
CONFIGURE_CFLAGS are used to run compiler for strerror_r returns char with gnu source is compilable detection.

In addition to the -O0 -Wall there comes _FORTIFY_SOURCE=3 which is set by default in the nix.

-O0 and _FORTIFY_SOURCE conflict with each other, there is a warning which causes strerror detection to fail.

Current revision also doesn't work on nix.

[th7nder]

Btw. I'm not sure whether _FORTIFY_SOURCE=3 should be set by default in nix. I was just trying to make this code agnostic of this kind of stuff, so should work no matter what people have in their compilers set.

[BusyJay]

so should work no matter what people have in their compilers set.

You can make one lib work with an incorrect configuration, but how do you make all libs with that? Don't people using Nix do any development? During development, it's quite common to use 0 optimization in debug build, how do you get around such warnings?

[th7nder]

I totally agree, that -O0 is quite common in the development and it shouldn't create conflicts.
I've got no idea why this started showing up recently.

[th7nder]

I did another investigation. __FORTIFY_SOURCE was also set on the older nix versions with gcc13, however it didn't fail the entire compilation. It didn't detect strerror properly, but it didn't fail, because gcc14 upgraded one of the warnings about the *char to int conversions an error.

x.c

#include <errno.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

int d() {
	char* a = NULL;
	return a;
}

int main() {
	return d();
}

gcc14

th7nder@stormheim ~/w/e/jemallocator (disable-warnings) [1]> gcc --version
gcc (GCC) 14.2.1 20241116
Copyright (C) 2024 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
th7nder@stormheim ~/w/e/jemallocator (disable-warnings) [1]> gcc x.c 
x.c: In function ‘d’:
x.c:8:16: error: returning ‘char *’ from a function with return type ‘int’ makes integer from pointer without a cast [-Wint-conversion]
    8 |         return a;

gcc13

th7nder@stormheim ~/w/e/jemallocator (disable-warnings)> gcc --version
gcc (GCC) 13.3.0
Copyright (C) 2023 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

th7nder@stormheim ~/w/e/jemallocator (disable-warnings)> gcc x.c
x.c: In function ‘d’:
x.c:8:16: warning: returning ‘char *’ from a function with return type ‘int’ makes integer from pointer without a cast [-Wint-conversion]
    8 |         return a;
      |                ^

[th7nder]

Given all of that, I think it's fair to say that if someone was using this library via compiler with __FORTIFY_SOURCE enabled (hopefully just Nix users (?)), they ended up using wrong strerror_t signature which may cause unintended consequences (?).

https://github.com/jemalloc/jemalloc/blob/257e64b968ec40c285331dfb6e3db8a2b34999d1/src/malloc_io.c#L99

[BusyJay]

they ended up using wrong strerror_t signature which may cause unintended consequences

Yes, this is issue jemallocator or jemalloc should take care of. An error should be reported instead of making wrong assumptions.

[BusyJay]

Now with latest change, it should report failure during dev build on Nix with _FORTIFY_SOURCE being set.

[th7nder]

gcc14 with _FORTIFTY_SOURCE=3 on nix

  checking whether strerror_r returns char with gnu source is compilable... no
  checking whether compile fail due to strerror_r signature is compilable... no
  
    src/malloc_io.c: In function ‘buferror’:
  src/malloc_io.c:107:16: error: returning ‘char *’ from a function with return type ‘int’ makes integer from pointer without a cast [-Wint-conversion]
    107 |         return strerror_r(err, buf, buflen);

gcc13 with _FORTIFY_SOURCE=3 on nix

  checking whether strerror_r returns char with gnu source is compilable... no
checking whether compile fail due to strerror_r signature is compilable... no

and the compilation succeeded.

So with those changes it still doesn't fail the build on gcc13 where strerror_r signature mismatch is treated as warning, not an error and produces invalid code.

[BusyJay]

A few lines were missing from the final configure file. And the detection description is updated to be readable.

[th7nder]

Can confirm, it gives an error with gcc13 now.

  checking whether strerror_r returns char with gnu source is compilable... no
  checking whether strerror_r header only is compilable... no
  running: "tail" "-n" "100" "/home/th7nder/workspace/eiger/jemallocator/target/debug/build/tikv-jemalloc-sys-917fd2e0db16ef1b/out/build/config.log"
  target_alias=''

  ## ----------- ##
  ## confdefs.h. ##
  ## ----------- ##

  /* confdefs.h */
  #define PACKAGE_NAME ""
  #define PACKAGE_TARNAME ""
  #define PACKAGE_VERSION ""
  #define PACKAGE_STRING ""
  #define PACKAGE_BUGREPORT ""
  #define PACKAGE_URL ""
  #define JEMALLOC_HAS_RESTRICT
  #define STDC_HEADERS 1
  #define HAVE_SYS_TYPES_H 1
  #define HAVE_SYS_STAT_H 1
  #define HAVE_STDLIB_H 1
  #define HAVE_STRING_H 1
  #define HAVE_MEMORY_H 1
  #define HAVE_STRINGS_H 1
  #define HAVE_INTTYPES_H 1
  #define HAVE_STDINT_H 1
  #define HAVE_UNISTD_H 1
  #define SIZEOF_VOID_P 8
  #define LG_SIZEOF_PTR 3
  #define SIZEOF_INT 4
  #define LG_SIZEOF_INT 2
  #define SIZEOF_LONG 8
  #define LG_SIZEOF_LONG 3
  #define SIZEOF_LONG_LONG 8
  #define LG_SIZEOF_LONG_LONG 3
  #define SIZEOF_INTMAX_T 8
  #define LG_SIZEOF_INTMAX_T 3
  #define HAVE_CPU_SPINWAIT 1
  #define CPU_SPINWAIT __asm__ volatile("pause")
  #define LG_VADDR 48
  #define LG_VADDR 48
  #define JEMALLOC_PURGE_MADVISE_DONTNEED_ZEROS  
  #define JEMALLOC_HAS_ALLOCA_H  
  #define JEMALLOC_PROC_SYS_VM_OVERCOMMIT_MEMORY  
  #define JEMALLOC_THREADED_INIT  
  #define JEMALLOC_USE_CXX_THROW  
  #define HAVE_MALLOC_H 1
  #define JEMALLOC_USABLE_SIZE_CONST 
  #define JEMALLOC_HAVE_ATTR  
  #define JEMALLOC_HAVE_ATTR_FALLTHROUGH  
  #define JEMALLOC_HAVE_ATTR_COLD  
  #define JEMALLOC_PREFIX "_rjem_"
  #define JEMALLOC_CPREFIX "_RJEM_"
  #define JEMALLOC_OVERRIDE_MEMALIGN  
  #define JEMALLOC_OVERRIDE_VALLOC  
  #define JEMALLOC_PRIVATE_NAMESPACE _rjem_je_
  #define JEMALLOC_CONFIG_MALLOC_CONF ""
  #define JEMALLOC_MAPS_COALESCE  
  #define JEMALLOC_RETAIN  
  #define JEMALLOC_ZERO_REALLOC_DEFAULT_FREE  
  #define JEMALLOC_DSS  
  #define JEMALLOC_FILL  
  #define JEMALLOC_CACHE_OBLIVIOUS  
  #define JEMALLOC_INTERNAL_UNREACHABLE __builtin_unreachable
  #define JEMALLOC_INTERNAL_FFSLL __builtin_ffsll
  #define JEMALLOC_INTERNAL_FFSL __builtin_ffsl
  #define JEMALLOC_INTERNAL_FFS __builtin_ffs
  #define JEMALLOC_INTERNAL_POPCOUNT __builtin_popcount
  #define JEMALLOC_INTERNAL_POPCOUNTL __builtin_popcountl
  #define JEMALLOC_INTERNAL_POPCOUNTLL __builtin_popcountll
  #define LG_PAGE 12
  #define LG_HUGEPAGE 21
  #define JEMALLOC_HAVE_PTHREAD  
  #define HAVE_PTHREAD_H 1
  #define HAVE_DLFCN_H 1
  #define JEMALLOC_HAVE_DLSYM  
  #define JEMALLOC_HAVE_PTHREAD_ATFORK  
  #define JEMALLOC_HAVE_PTHREAD_SETNAME_NP  
  #define JEMALLOC_HAVE_PTHREAD_GETNAME_NP  
  #define JEMALLOC_HAVE_CLOCK_MONOTONIC_COARSE  
  #define JEMALLOC_HAVE_CLOCK_MONOTONIC  
  #define JEMALLOC_HAVE_CLOCK_REALTIME  
  #define JEMALLOC_HAVE_SECURE_GETENV  
  #define JEMALLOC_HAVE_SCHED_GETCPU  
  #define JEMALLOC_HAVE_SCHED_SETAFFINITY  
  #define JEMALLOC_TLS
  #define JEMALLOC_C11_ATOMICS  
  #define JEMALLOC_GCC_ATOMIC_ATOMICS  
  #define JEMALLOC_GCC_U8_ATOMIC_ATOMICS  
  #define JEMALLOC_GCC_SYNC_ATOMICS  
  #define JEMALLOC_GCC_U8_SYNC_ATOMICS  
  #define JEMALLOC_HAVE_MADVISE  
  #define JEMALLOC_PURGE_MADVISE_FREE  
  #define JEMALLOC_PURGE_MADVISE_DONTNEED  
  #define JEMALLOC_MADVISE_DONTDUMP  
  #define JEMALLOC_HAVE_MADVISE_HUGE  
  #define JEMALLOC_HAVE_MPROTECT  
  #define JEMALLOC_HAVE_BUILTIN_CLZ  
  #define JEMALLOC_TLS_MODEL __attribute__((tls_model("initial-exec")))
  #define JEMALLOC_BACKGROUND_THREAD  
  #define JEMALLOC_HAVE_PTHREAD_MUTEX_ADAPTIVE_NP  

  configure: exit 1

  --- stderr
  configure: error: cannot determine return type of strerror_r
  thread 'main' panicked at jemalloc-sys/build.rs:388:9:
  command did not execute successfully: cd "/home/th7nder/workspace/eiger/jemallocator/target/debug/build/tikv-jemalloc-sys-917fd2e0db16ef1b/out/build" && CC="gcc" CFLAGS="-O0 -ffunction-sections -fdata-sections -fPIC -gdwarf-4 -fno-omit-frame-pointer -m64 -Wall" CPPFLAGS="-O0 -ffunction-sections -fdata-sections -fPIC -gdwarf-4 -fno-omit-frame-pointer -m64 -Wall" LDFLAGS="-O0 -ffunction-sections -fdata-sections -fPIC -gdwarf-4 -fno-omit-frame-pointer -m64 -Wall" "sh" "/home/th7nder/workspace/eiger/jemallocator/target/debug/build/tikv-jemalloc-sys-917fd2e0db16ef1b/out/build/configure" "--with-version=5.3.0-1-ge13ca993e8ccb9ba9847cc330696e02839f328f7" "--disable-cxx" "--enable-doc=no" "--enable-shared=no" "--with-jemalloc-prefix=_rjem_" "--with-private-namespace=_rjem_" "--disable-stats" "--host=x86_64-unknown-linux-gnu" "--build=x86_64-unknown-linux-gnu" "--prefix=/home/th7nder/workspace/eiger/jemallocator/target/debug/build/tikv-jemalloc-sys-917fd2e0db16ef1b/out"
  expected success, got: exit status: 1
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace