Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libc-wrappers: Ensure binaries built on Fedora 35 run on older Fedoras #829

Closed
wants to merge 4 commits into from

Conversation

olivergs
Copy link
Collaborator

@olivergs olivergs commented Jul 5, 2021

@olivergs olivergs force-pushed the wrap-pthread-glibc2.34 branch from b678d1f to 27605eb Compare July 5, 2021 15:03
@softwarefactory-project-zuul
Copy link

Build failed.

@softwarefactory-project-zuul
Copy link

Build failed.

@HarryMichal HarryMichal added 2. Under The Hood Multiple areas of the app are influenced by this ticket 3. Bugfix Fixes a bug 6. Minor Change Should not cause breakage labels Jul 7, 2021
@HarryMichal
Copy link
Member

recheck

@softwarefactory-project-zuul
Copy link

Build failed.

@HarryMichal
Copy link
Member

recheck

@HarryMichal
Copy link
Member

This successfully works on my Fedora Silverblue Rawhide machine with glibc-2.33.9000-40.fc35.x86_64 on the host and glibc-2.33.9000-2.fc35.x86_64 inside of a toolbox container.

@softwarefactory-project-zuul
Copy link

Build succeeded.

@HarryMichal
Copy link
Member

Please, rebase without the test because it has been separated into #831

Copy link
Member

@HarryMichal HarryMichal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The wrapping seems to do the job, so let's ship it! Just make sure to rebase without the test.

@olivergs
Copy link
Collaborator Author

The wrapping seems to do the job, so let's ship it! Just make sure to rebase without the test.

Seems I used a different branch and put the test there by accident. Rebasing.

@olivergs olivergs force-pushed the wrap-pthread-glibc2.34 branch from f77b68a to 27605eb Compare July 16, 2021 16:43
@softwarefactory-project-zuul
Copy link

Build succeeded.

@theotheroracle
Copy link

anything preventing this from merging rn ?

@olivergs
Copy link
Collaborator Author

We are working on reviewing it and checking alternative approaches to fix this.

A subsequent commit will add more wrappers to cover new symbol versions
introduced in glibc-2.34. This will make it easier to read because it
won't disrupt the rest of the build parameters.

containers#821
Copy link
Member

@debarshiray debarshiray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this, @olivergs !

It would be good to mention the user-visible problem in the commit message, because the names of the wrapped functions is somewhat obvious from just looking at the commit. eg., the version of Fedora or glibc that triggered this issue.

src/libc-wrappers/libc-wrappers.c Outdated Show resolved Hide resolved
@debarshiray debarshiray force-pushed the wrap-pthread-glibc2.34 branch from 27605eb to 95c1239 Compare September 13, 2021 22:57
@debarshiray debarshiray changed the title libc-wrappers: Added wrappers for __libc_start_main, pthread_attr_get… libc-wrappers: Ensure binaries built on Fedora 35 run on older Fedoras Sep 13, 2021
@softwarefactory-project-zuul
Copy link

Build succeeded.

Copy link
Member

@debarshiray debarshiray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will be good to have at least some references to the glibc changes in our commit message. Otherwise, it's difficult for the uninitiated to validate the changes.

src/libc-wrappers/libc-wrappers.c Outdated Show resolved Hide resolved
@debarshiray debarshiray force-pushed the wrap-pthread-glibc2.34 branch from 95c1239 to 17e2ab7 Compare September 14, 2021 00:45
@softwarefactory-project-zuul
Copy link

Build succeeded.

A subsequent commit will add more wrappers to cover new symbol versions
introduced in glibc-2.34. This will make it easier to read by clearly
segregating the file into separate sections for each wrapper.

containers#821
src/libc-wrappers/libc-wrappers.c Outdated Show resolved Hide resolved
src/libc-wrappers/libc-wrappers.c Outdated Show resolved Hide resolved
Recently, glibc-2.34, which is used by Fedora 35 onwards, added new
versions of the pthread_attr_getstacksize [1], pthread_create [2] and
pthread_detach [3] symbols as part of the libpthread removal
project [4]:
  $ objdump -T /usr/bin/toolbox | grep GLIBC_2.34
  ...
  0000000000000000      DF *UND*	0000000000000000  GLIBC_2.34
    pthread_detach
  0000000000000000      DF *UND*	0000000000000000  GLIBC_2.34
    pthread_create
  0000000000000000      DF *UND*	0000000000000000  GLIBC_2.34
    pthread_attr_getstacksize

This means that /usr/bin/toolbox binaries built against glibc-2.34 on
newer Fedoras pick up the latest version of the symbols and fail to run
against older glibcs in older Fedoras.

[1] glibc commit ee092efed40d667b
    https://sourceware.org/git/?p=glibc.git;a=commit;h=ee092efed40d667b

[2] glibc commit f47f1d91af985a90
    https://sourceware.org/git/?p=glibc.git;a=commit;h=f47f1d91af985a90

[3] glibc commit df65f897e9501aa5
    https://sourceware.org/git/?p=glibc.git;a=commit;h=df65f897e9501aa5

[4] https://sourceware.org/ml/libc-alpha/2019-10/msg00080.html

containers#821
@debarshiray debarshiray force-pushed the wrap-pthread-glibc2.34 branch from 17e2ab7 to 216314d Compare September 16, 2021 15:45
@softwarefactory-project-zuul
Copy link

Build succeeded.

@HarryMichal
Copy link
Member

This fix is not complete. Trying to launch a toolbox container with this patch applied generates the following error message in the container entry-point:

toolbox: symbol lookup error: toolbox: undefined symbol: pthread_attr_getstacksize, version GLIBC_2.2.5

Either we try to fine-tune this patch or we'll have to use a different approach to fix this issue.

@HarryMichal HarryMichal added the 5. Help Wanted Extra attention is needed label Oct 19, 2021
@HarryMichal
Copy link
Member

recheck

@softwarefactory-project-zuul
Copy link

Build failed.

@debarshiray
Copy link
Member

This approach of restricting the versions of the glibc symbols that are linked against isn't actually supported by glibc, and breaks if the early process start-up code changes. That's why it doesn't work with glibc-2.34, which is used by Fedora 35 onwards, because a new version of the __libc_start_main symbol was added as part of some security hardening.

@debarshiray
Copy link
Member

Let's try #897

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. Under The Hood Multiple areas of the app are influenced by this ticket 3. Bugfix Fixes a bug 5. Help Wanted Extra attention is needed 6. Minor Change Should not cause breakage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants