Fix libxcrypt algorithm detection in config.m4 #1

zeriyoshi · 2024-08-29T15:17:38Z

The current config.m4 appears to be unable to properly detect libxcrypt algorithms.

I believe the environment variable that should be set is LIBS, not LDFLAGS.

I tested it on Debian using the following Dockerfile:

ARG PLATFORM=${BUILDPLATFORM:-linux/amd64}
ARG IMAGE=php
ARG TAG=8.3-cli-bookworm

FROM --platform=${PLATFORM} ${IMAGE}:${TAG}

ENV USE_ZEND_ALLOC=0
ENV USE_TRACKED_ALLOC=1
ENV ZEND_DONT_UNLOAD_MODULES=1
ENV LC_ALL="C"

RUN docker-php-source extract \
 && if test -f "/etc/debian_version"; then \
      echo "deb http://apt.llvm.org/bookworm/ llvm-toolchain-bookworm main" > "/etc/apt/sources.list.d/llvm.list" \
 &&   echo "deb-src http://apt.llvm.org/bookworm/ llvm-toolchain-bookworm main" >> "/etc/apt/sources.list.d/llvm.list" \
 &&   curl -fsSL "https://apt.llvm.org/llvm-snapshot.gpg.key" -o "/etc/apt/trusted.gpg.d/apt.llvm.org.asc" \
 &&   apt-get update \
 &&   DEBIAN_FRONTEND="noninteractive" apt-get install -y "bison" "re2c" "zlib1g-dev" "libsqlite3-dev" "libxml2-dev" \
        "autoconf" "pkg-config" "make" "gcc" "valgrind" "rsync" "git" "ssh" \
        "clang-20" \
        "lcov" "gzip" \
        "vim" \
 &&   update-alternatives --install "/usr/bin/clang" clang "/usr/bin/clang-20" 100 \
 &&   update-alternatives --install "/usr/bin/clang++" clang++ "/usr/bin/clang++-20" 100; \
    else \
      apk add --no-cache "bison" "zlib-dev" "sqlite-dev" "libxml2-dev" \
        "autoconf" "pkgconfig" "make" "gcc" "g++" "valgrind" "valgrind-dev" \
        "musl-dev" "rsync" "git" "openssh" \
        "patch" "lcov" "gzip" \
        "vim"; \
    fi

COPY ./pskel.sh /usr/local/bin/pskel
COPY ./patches /patches
COPY ./ext /ext

# ----

ARG CFLAGS=""

RUN if test -f "/etc/debian_version"; then \
      apt-get update && apt-get install -y "libtool"; \
    else \
      apk add --no-cache "libtool"; \
    fi \
 && git clone --depth=1 --branch="v4.4.36" "https://github.com/besser82/libxcrypt.git" "/libxcrypt" \
 && cd "/libxcrypt" \
 &&   ./autogen.sh \
 &&   CFLAGS="${CFLAGS}" ./configure --enable-hashes="all" \
 &&   make -j"$(nproc)" \
 &&   make install \
 && cd -

I discovered this while making it testable in the PHP Extension skeleton I'm creating.

https://github.com/zeriyoshi/pskel

It can run tests on Debian / Alpine / Windows using GitHub Actions and calculate coverage.

If you're interested in this skeleton, please let me know :)

remicollet · 2024-08-30T05:47:48Z

Using AC_LINK_IFELSE doesn't work, as detection needs AC_RUN_IFELSE

using LDFLAGS should work (but there is an error mixing CFLAGS there)

Please try after e7a9d75

remicollet · 2024-08-30T07:56:36Z

If fix is OK, I will release a RC2 shortly

zeriyoshi · 2024-08-30T15:25:17Z

@remicollet
Thank you! I tried it, but it seems that additions to LIBS are indeed necessary. I've made minimal changes to make it work correctly. How does it look?

If you're interested in CI for GitHub Actions, I can create a separate PR for that :)

remicollet · 2024-08-30T15:50:41Z

Please post config.log (failing part with build sources and command)

zeriyoshi · 2024-08-30T16:00:26Z

@remicollet
~~I simply ./configure, I guess I forgot to do that. My apologies.~~
~~This appears to be working correctly!~~

configure:4741: checking for yescrypt
configure:4762: cc -o conftest -g -O2 -I/usr/local/include    conftest.c -L/usr/local/lib -lcrypt  >&5
configure:4762: $? = 0
configure:4765: result: available
configure:4780: checking for sha512 algo
configure:4801: cc -o conftest -g -O2 -I/usr/local/include    conftest.c -L/usr/local/lib -lcrypt  >&5
configure:4801: $? = 0
configure:4807: result: available

zeriyoshi · 2024-08-30T16:13:46Z

No, it still doesn't seem to work. The config.log is as follows

configure:4785: checking for yescrypt
configure:4809: cc -o conftest  -I/usr/local/include   -L/usr/local/lib -lcrypt   conftest.c  >&5
/usr/bin/ld: /tmp/ccI3LMBt.o: in function `main':
conftest.c:(.text+0x34): undefined reference to `crypt_gensalt'
collect2: error: ld returned 1 exit status
configure:4809: $? = 1
configure: program exited with status 1
configure: failed program was:
| /* confdefs.h */
| #define PACKAGE_NAME ""
| #define PACKAGE_TARNAME ""
| #define PACKAGE_VERSION ""
| #define PACKAGE_STRING ""
| #define PACKAGE_BUGREPORT ""
| #define PACKAGE_URL ""
| /* end confdefs.h.  */
| 
| #include <string.h>
| #include <unistd.h>
| #include <crypt.h>
| #include <stdlib.h>
| #include <string.h>
| 
| int main(void) {
|     char salt[8];
| 	salt[0]='$'; salt[1]='y'; salt[2]='$'; salt[3]=0;
| 	return crypt_gensalt(salt, 0, NULL, 0) ? 0 : 1;
| }
configure:4820: result: missing
configure:4829: checking for sha512 algo
configure:4853: cc -o conftest  -I/usr/local/include   -L/usr/local/lib -lcrypt   conftest.c  >&5
/usr/bin/ld: /tmp/ccWp1aut.o: in function `main':
conftest.c:(.text+0x34): undefined reference to `crypt_gensalt'
collect2: error: ld returned 1 exit status
configure:4853: $? = 1
configure: program exited with status 1
configure: failed program was:
| /* confdefs.h */
| #define PACKAGE_NAME ""
| #define PACKAGE_TARNAME ""
| #define PACKAGE_VERSION ""
| #define PACKAGE_STRING ""
| #define PACKAGE_BUGREPORT ""
| #define PACKAGE_URL ""
| /* end confdefs.h.  */
| 
| #include <string.h>
| #include <unistd.h>
| #include <crypt.h>
| #include <stdlib.h>
| #include <string.h>
| 
| int main(void) {
|     char salt[8];
| 	salt[0]='$'; salt[1]='6'; salt[2]='$'; salt[3]=0;
| 	return crypt_gensalt(salt, 0, NULL, 0) ? 0 : 1;
| }
configure:4864: result: missing

zeriyoshi · 2024-08-30T16:15:16Z

It worked correctly with only the following modifications. Does this cause unintended behavior in EL?

diff --git a/config.m4 b/config.m4
index f1fcb05..3f702c3 100644
--- a/config.m4
+++ b/config.m4
@@ -13,6 +13,7 @@ if test "$PHP_XPASS" != "no"; then
 
   old_CFLAGS=$CFLAGS; CFLAGS="$CFLAGS $LIBXCRYPT_CFLAGS"
   old_LDFLAGS=$LDFLAGS; LDFLAGS="$LIBXCRYPT_LIBS $LDFLAGS"
+  old_LIBS=$LIBS; LIBS="$LIBS $LIBXCRYPT_LIBS"
 
   AC_MSG_CHECKING([for yescrypt])
   AC_RUN_IFELSE([AC_LANG_SOURCE([[

remicollet · 2024-08-30T16:18:24Z

It worked correctly with only the following modifications. Does this cause unintended behavior in EL?

I'd like to understand why command is bad for you and OK for me

configure:4853: cc -o conftest -I/usr/local/include -L/usr/local/lib -lcrypt conftest.c >&5

Should be

configure:4853: cc -o conftest -I/usr/local/include -L/usr/local/lib conftest.c -lcrypt >&5

P.S. and adding both LDFLAGS and LIBS add the library twice...

zeriyoshi · 2024-08-30T16:33:23Z

Can you tell us about your environment so I can investigate? Is it due to the compiler version...?

I ran CI. For now, sha512 and yescrypt are disabled on Debian / Alpine due to similar issues.
zeriyoshi#1

zeriyoshi · 2024-08-30T16:40:20Z

This is probably due to the gcc version. It works fine in clang.

NG: CC=gcc ./configure && make clean && TEST_PHP_ARGS="-q" make -j12 test
OK: CC=clang ./configure && make clean && TEST_PHP_ARGS="-q" make -j12 test

$ gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/lib/gcc/aarch64-linux-gnu/12/lto-wrapper
Target: aarch64-linux-gnu
Configured with: ../src/configure -v --with-pkgversion='Debian 12.2.0-14' --with-bugurl=file:///usr/share/doc/gcc-12/README.Bugs --enable-languages=c,ada,c++,go,d,fortran,objc,obj-c++,m2 --prefix=/usr --with-gcc-major-version-only --program-suffix=-12 --program-prefix=aarch64-linux-gnu- --enable-shared --enable-linker-build-id --libexecdir=/usr/lib --without-included-gettext --enable-threads=posix --libdir=/usr/lib --enable-nls --enable-clocale=gnu --enable-libstdcxx-debug --enable-libstdcxx-time=yes --with-default-libstdcxx-abi=new --enable-gnu-unique-object --disable-libquadmath --disable-libquadmath-support --enable-plugin --enable-default-pie --with-system-zlib --enable-libphobos-checking=release --with-target-system-zlib=auto --enable-objc-gc=auto --enable-multiarch --enable-fix-cortex-a53-843419 --disable-werror --enable-checking=release --build=aarch64-linux-gnu --host=aarch64-linux-gnu --target=aarch64-linux-gnu
Thread model: posix
Supported LTO compression algorithms: zlib zstd
gcc version 12.2.0 (Debian 12.2.0-14)

remicollet · 2024-08-31T05:08:40Z

I only use GCC (8.5, 11.5, 13.3, 14.2)

I run the build on Fedora 39, 40, 41 and RHEL 8, 9 against PHP 8.0 to 8.4

zeriyoshi · 2024-08-31T08:23:17Z

I've confirmed that using LIBS instead of LDFLAGS with the following configuration works as intended. The Dockerfile and commands used are available in the repository below.

https://github.com/zeriyoshi/php-xpass-tester

EL8 / EL9 result: https://github.com/zeriyoshi/php-xpass-tester/actions/runs/10643794080

With this configuration, I believe it will work correctly on both Debian and Alpine Linux.

remicollet · 2024-09-01T05:41:38Z

config.m4

@@ -12,15 +12,15 @@ if test "$PHP_XPASS" != "no"; then
  PHP_EVAL_LIBLINE([$LIBXCRYPT_LIBS], [XPASS_SHARED_LIBADD])

  old_CFLAGS=$CFLAGS; CFLAGS="$CFLAGS $LIBXCRYPT_CFLAGS"
-  old_LDFLAGS=$LDFLAGS; LDFLAGS="$LIBXCRYPT_LIBS $LDFLAGS"
+  old_LDFLAGS=$LDFLAGS; LDFLAGS="$LDFLAGS"


Please drop this no-effect line

config.m4

remicollet · 2024-09-01T05:43:00Z

This is quite strange, in php-src various ext use LIBS (curl, gd...) others use LDFLAGS (iconv)

remicollet · 2024-09-01T05:44:59Z

@petk, perhaps you can help here with AC_RUN_IF_ELSE / LDFLAGS / LIBS ?

petk · 2024-09-01T06:26:35Z

Hm, yes it seems that LIBS is a better place to append the $LIBXCRYPT_LIBS. ~~Somehow on Debian~~ (with GCC) it needs to be passed after the test source file and then it find them.

# here is undefined reference error:
gcc -lcrypt conftest.c 
# this works ok:
gcc conftest.c -lcrypt

Yes, the changes in the PR are ok. And those LDFLAGS adjustments are redundant as already noted.

When building: ./configure --with-iconv=shared,/path/to/libiconv the iconv couldn't be found due to linker error Autoconf places LDFLAGS before the conftest.c file in the test compile command and LIBS after it. GCC also requires this. Similar issue discovered at remicollet/php-xpass#1

zeriyoshi · 2024-09-01T11:08:07Z

@remicollet @petk
OK, I fixed it :)

remicollet · 2024-09-01T15:40:28Z

Squashed, amended and merged in 97bfb67

Thanks a lot

When building iconv as shared and with external library (for example, libiconv): ./configure --with-iconv=shared,/path/to/libiconv the iconv couldn't be found due to a linker error. Autoconf places LDFLAGS before the conftest.c file in the test compile command and LIBS after it. GCC also requires this: gcc -L... conftest.c -liconv Similar issue discovered at remicollet/php-xpass#1

zeriyoshi force-pushed the hotfix_autoconf branch from 8ff0926 to 586d449 Compare August 30, 2024 15:22

zeriyoshi added 3 commits August 30, 2024 15:23

fix config.m4

c70b1d1

fix message

726dadd

fix

3a848da

zeriyoshi force-pushed the hotfix_autoconf branch from 586d449 to 3a848da Compare August 30, 2024 15:23

fix twice option

be9c7e6

remicollet reviewed Sep 1, 2024

View reviewed changes

remicollet requested changes Sep 1, 2024

View reviewed changes

config.m4 Show resolved Hide resolved

petk mentioned this pull request Sep 1, 2024

Autotools: Fix iconv shared build with external library php/php-src#15686

Merged

fix unused LDFLAGS definition

42cf224

remicollet closed this Sep 1, 2024

zeriyoshi deleted the hotfix_autoconf branch September 2, 2024 00:11

Fix libxcrypt algorithm detection in config.m4 #1

Fix libxcrypt algorithm detection in config.m4 #1

Uh oh!

Conversation

zeriyoshi commented Aug 29, 2024

Uh oh!

remicollet commented Aug 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

remicollet commented Aug 30, 2024

Uh oh!

zeriyoshi commented Aug 30, 2024

Uh oh!

remicollet commented Aug 30, 2024

Uh oh!

zeriyoshi commented Aug 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zeriyoshi commented Aug 30, 2024

Uh oh!

zeriyoshi commented Aug 30, 2024

Uh oh!

remicollet commented Aug 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zeriyoshi commented Aug 30, 2024

Uh oh!

zeriyoshi commented Aug 30, 2024

Uh oh!

remicollet commented Aug 31, 2024

Uh oh!

zeriyoshi commented Aug 31, 2024

Uh oh!

remicollet Sep 1, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

remicollet commented Sep 1, 2024

Uh oh!

remicollet commented Sep 1, 2024

Uh oh!

petk commented Sep 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zeriyoshi commented Sep 1, 2024

Uh oh!

remicollet commented Sep 1, 2024

Uh oh!

Uh oh!

remicollet commented Aug 30, 2024 •

edited

Loading

zeriyoshi commented Aug 30, 2024 •

edited

Loading

remicollet commented Aug 30, 2024 •

edited

Loading

petk commented Sep 1, 2024 •

edited

Loading