conan-io / hooks

Official Conan client hooks
MIT License
32 stars 44 forks source link

[conan-center] Forbid linking against removed glibc finite math functions #508

Closed valgur closed 6 months ago

valgur commented 11 months ago

Adds a linter check for any __*_finite symbols in .a, .so and executable files on Linux to avoid distributing binaries that link against glibc symbols that are no longer available on newer systems with glibc v2.31+. This affects quite a few audio and image libraries that have --ffast-math set, such as mpg123, openjpeg, libx264 and libx265.

See https://github.com/conan-io/conan-center-index/issues/18951#issuecomment-1662472084 for context.

Here are also some code snippets to find any affected libraries in a local Conan cache: https://gist.github.com/valgur/d3b1576de71d29467e2cd2434e19af02 And a full list of glibc symbols this check is supposed to catch: https://gist.github.com/valgur/dac1a98174df755cfa85a04fcc5a93a1

valgur commented 11 months ago

Here's a Dockerfile to verify that the new hook catches the issue on Ubuntu 18.04 / glibc 2.27:

FROM conanio/gcc8:1.60.2

USER root
RUN git clone https://github.com/conan-io/conan-center-index.git --depth 1
RUN conan profile new default --detect && conan profile update settings.compiler.libcxx=libstdc++11 default
RUN mkdir -p ~/.conan/hooks/ && \
    wget https://raw.githubusercontent.com/valgur/conan-hooks/feature/forbid-glibc-finite-math-functions/hooks/conan-center.py -O ~/.conan/hooks/conan-center.py && \
    chmod +x ~/.conan/hooks/conan-center.py
RUN conan config set hooks.conan-center
RUN conan create conan-center-index/recipes/mpg123/all /1.31.2@ --build=mpg123 -pr:b=default
RUN objdump -t ~/.conan/data/mpg123/1.31.2/_/_/package/*/lib/libmpg123.a | grep _finite || true

# Uncomment to verify that building with the offending optimization turned off fixes the issue
# RUN CFLAGS=-fno-finite-math-only conan create conan-center-index/recipes/mpg123/all /1.31.2@ --build=mpg123 -pr:b=default
# RUN objdump -t ~/.conan/data/mpg123/1.31.2/_/_/package/*/lib/libmpg123.a | grep _finite || true

Which produces:

Step 7/10 : RUN conan create conan-center-index/recipes/mpg123/all /1.31.2@ --build=mpg123 -pr:b=default
[...]
mpg123/1.31.2 (test package): Running test()
WARN: Remotes registry file missing, creating default one in /root/.conan/remotes.json
[HOOK - conan-center.py] post_package(): ERROR: [USING GLIBC FINITE MATH FUNCTIONS (KB-H078)] Package contains files that link against functions removed from glibc >= v2.31:
bin/mpg123-id3dump: __exp2_finite, __exp_finite, __log_finite
bin/mpg123-strip: __exp2_finite, __exp_finite, __log_finite
bin/mpg123: __exp2_finite, __exp_finite, __log10_finite, __log_finite
bin/out123: __exp_finite, __log10_finite, __log_finite
lib/libmpg123.a: __exp2_finite, __exp_finite, __exp_finite, __log_finite
lib/libsyn123.a: _ZGVbN2v___exp_finite, __exp_finite, __exp_finite, __log10_finite, __log_finite
Please add -fno-finite-math-only to compiler flags to disable the use of these functions in optimizations. (https://github.com/conan-io/conan-center-index/blob/master/docs/error_knowledge_base.md#KB-H078-USING-GLIBC-FINITE-MATH-FUNCTIONS) 
 ---> e704ef8f3151
Step 8/8 : RUN objdump -t ~/.conan/data/mpg123/1.31.2/_/_/package/*/lib/libmpg123.a | grep _finite || true
 ---> Running in 861d11ead80f
0000000000000000         *UND*  0000000000000000 __exp_finite
0000000000000000         *UND*  0000000000000000 __log_finite
0000000000000000         *UND*  0000000000000000 __exp_finite
0000000000000000         *UND*  0000000000000000 __exp2_finite
 ---> Running in 1f07aaba5626
Step 9/10 : RUN CFLAGS=-fno-finite-math-only conan create conan-center-index/recipes/mpg123/all /1.31.2@ --build=mpg123 -pr:b=default
[...]
mpg123/1.31.2 (test package): Running test()
 ---> e6eebcae5af7
Step 10/10 : RUN objdump -t ~/.conan/data/mpg123/1.31.2/_/_/package/*/lib/libmpg123.a | grep _finite || true
 ---> Running in 41ce466a9cae
valgur commented 11 months ago

@uilianries, @memsharded. Have you had a chance to look at this PR? A friendly ping just in case this has slipped through the cracks.

By my count this issue affects at least 75 packages just through the transitive dependencies on mpg123, openjpeg, libx264 and libx265 alone. I would like to add the one-liner fixes to these packages and others but would like this hook to be merged first to be certain that the changes are really having an effect.

uilianries commented 11 months ago

I'm concerned about this hook, just like @memsharded said, it's complex and hard to understand, and can turn up as a nightmare in case we need to fix something in the future.

Problems related to glibc version in CCI are not new and we really can't fix all them. If we use only new Ubuntu versions, we will break users that are still running older versions, but if we only run old versions, we private some libraries due new glibc features, so we try to keep it balanced. Our suggestion is building from source in case having trouble with glibc, so you can update your package according to your scenario. But in case of real failure in CCI, we should investigate for sure.

valgur commented 11 months ago

I am a bit concerned about the complexity of this hook check

I'm concerned about this hook, just like @memsharded said, it's complex and hard to understand, and can turn up as a nightmare in case we need to fix something in the future.

While I empathize with the maintenance concern, I based this hook largely on "KB-H043 - MISSING SYSTEM LIBS" and feel like this hook is almost straightforward compared to that one?

Problems related to glibc version in CCI are not new and we really can't fix all them.

Are there any other examples besides this one?

I find it hard to agree if the fix for the issue at hand is to add the following to couple of lines to generate()

if not is_msvc(self):
    tc.variables["CMAKE_C_FLAGS"] = "-fno-finite-math-only"
    tc.variables["CMAKE_CXX_FLAGS"] = "-fno-finite-math-only"

as long as there's a simple way to detect this issue (which there isn't when building locally - you would have to run something similar to the above Dockerfile for each recipe to even detect the issue).

Not detecting and fixing this issue effectively means that almost none of the packages involving some kind of image or audio I/O can be built with GCC (new or old) unless everything is rebuilt locally. This includes Qt, GTK, OpenCV, GDAL, SDL and many other packages that are both popular and a pain to re-build. Why bother distributing the binaries at all if they will only work with 5+ year old OS releases?

valgur commented 11 months ago

And as for

possible risks of false positives

It only matches against undefined symbols in library files with two leading underscores. The language standard only allows these to be defined by the compiler and the libc itself. There's a very limited number of these around. The regex pattern could also be replaced with one that matches the exact list of relevant symbols that I linked to above.

There is one case where this hook could incorrectly flag an issue, though - pre-built external binaries. I probably should add an exception for that. Edit: added an exception for this.

valgur commented 9 months ago

@memsharded @uilianries @jcar87 Please. I understand that you would prefer to not touch the hooks code at all, if possible. So I propose the following:

  1. Run this test to confirm that the hook works as expected:
    
    FROM conanio/gcc8:1.60.2

Set-up

USER root RUN git clone https://github.com/conan-io/conan-center-index.git --depth 1 RUN conan profile new default --detect && conan profile update settings.compiler.libcxx=libstdc++11 default RUN mkdir -p ~/.conan/hooks/ && \ wget https://raw.githubusercontent.com/valgur/conan-hooks/feature/forbid-glibc-finite-math-functions/hooks/conan-center.py -O ~/.conan/hooks/conan-center.py && \ chmod +x ~/.conan/hooks/conan-center.py RUN conan config set hooks.conan-center

The actual test

RUN conan create conan-center-index/recipes/mpg123/all /1.31.2@ --build=mpg123 -pr:b=default

Uncomment to verify that building with the offending optimization turned off fixes the issue

RUN CFLAGS=-fno-finite-math-only conan create conan-center-index/recipes/mpg123/all /1.31.2@ --build=mpg123 -pr:b=default


2. Merge.
3. Revert the PR if there's a slightest hint of issues in CCI related to this hook.
4. If you ever decide to migrate hooks to Conan v2, feel free to drop this one entirely. Any packages for which this hook is relevant are most likely fixed by then anyway.

Would that be an acceptable compromise?
valgur commented 9 months ago

Cc @SpaceIm @ericLemanissier @toge @jwillikers Do you agree that this is a real issue in CCI that needs a fix?

ericLemanissier commented 9 months ago

I agree that it's a real issue, which somehow needs to be addressed. Unfortunately, the current policy seems to be to reduce all automation changes on conan-center to the bare minimum. If this does not get merged as a hook, It'll probably has more chances to live as a custom command which would have the advantage of being compatible with conan 2. Then you'll have to find some way to make this run automatically on conan-center binaries (cf my DM on slack)

jwillikers commented 9 months ago

@valgur I have to agree that this is an important fix for making Conan Center Index binaries consumable by others on Linux. It should not be overlooked as it can lead to unexpected and hard to debug errors for consumers.

Unfortunately, libc handling has always been an omission with Conan. I've solved this kind of issue by using a subsetting of os for the libc type and libc version. By gatekeeping and rebuilding all binaries from CCI, this setting is always added to the Conan packages from CCI. It's not ideal since it doesn't allow using something compiled against an older glibc on a system with a newer glibc, but I guess it has the benefit of preventing somewhat rarer issues like this one.

I hope the team is able to figure out ways to improve glibc handling, like what your PR offers here. Thanks @valgur for working on this.

AbrilRBS commented 9 months ago

Thanks for your insight everyone :) I've set this to be discussed with the team, thanks a lot for your patience, will let you know once I know more!