Bug 14140 - Overlinking libreplace against librt and pthread against every binary or library causes issues
Overlinking libreplace against librt and pthread against every binary or libr...
Status: RESOLVED FIXED
Product: Samba 4.1 and newer
Classification: Unclassified
Component: Build
unspecified
All All
: P5 normal
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2019-09-24 08:19 UTC by Andreas Schneider
Modified: 2019-11-05 08:52 UTC (History)
9 users (show)

See Also:


Attachments
patch for 4.11 (12.71 KB, patch)
2019-09-26 10:16 UTC, Andreas Schneider
ab: review+
Details
patch for 4.10 (12.71 KB, patch)
2019-09-26 10:16 UTC, Andreas Schneider
ab: review+
Details
wip patch for stress-nss-libwbclient (784 bytes, patch)
2019-10-15 11:05 UTC, Isaac Boukris
no flags Details
additional patch for 4.11 (2.02 KB, patch)
2019-10-16 11:28 UTC, Andreas Schneider
metze: review+
Details
new patch for 4.10 (14.74 KB, patch)
2019-10-16 11:30 UTC, Andreas Schneider
metze: review+
Details
additional patch for 4.10 to remove shmget and shm_open checks (2.72 KB, patch)
2019-10-22 14:50 UTC, Andreas Schneider
ab: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Schneider 2019-09-24 08:19:39 UTC
Overlinking:

Currently libreplace links against librt, but it isn't needed as the symbols we are looking for are provided by glibc.

The replace waf script checks for pthread and sets LDFLAGS=-pthread globally. However most of our code does not need this at all.


Impact on NSS modules:

NSS modules are more fragile and during and update they can be non-functional for some time. Like with nss-winbind it wont work till all libraries have been upgraded and winbind installed and restarted.

During an upgrade the libnss_winbind and libnss_wins, known as the
"winbind" and "wins" service providers for NSS, can trigger a glibc defect
which causes the upgrade to fail. The reason is that librt and libpthread are marked as NODELETE so they wont be reloaded when a new nss_winbind has been installed.

We can avoid the glibc bug if we link those modules only against strictly necessary libraries. nss_winbind and nss_wins don't have to be linked against librt nor libpthread at all.

RH Samba bug: https://bugzilla.redhat.com/show_bug.cgi?id=1754575
RH glibc bug: https://bugzilla.redhat.com/show_bug.cgi?id=1748197
Comment 1 Mathieu Parent 2019-09-25 19:39:29 UTC
This was (probably) changed in 4.11 by ea7231dcc0b50c535f913f0542d600d0b2119a21. "libwbclient: Use wrapper for string to integer conversion".


Side note: I remember Ubuntu thinking about making libwbclient statically linked (but couldn't find the link).
Comment 2 Isaac Boukris 2019-09-26 09:05:08 UTC
Note that we dropped the nss_wins part as we couldn't avoid pthread there since it depends on samba-util library which needs it (it also depends on krb5 libs which are linked against pthread).
Comment 3 Andreas Schneider 2019-09-26 10:16:14 UTC
Created attachment 15492 [details]
patch for 4.11
Comment 4 Andreas Schneider 2019-09-26 10:16:33 UTC
Created attachment 15493 [details]
patch for 4.10
Comment 5 Alexander Bokovoy 2019-09-26 12:15:18 UTC
Comment on attachment 15492 [details]
patch for 4.11

LGTM
Comment 6 Alexander Bokovoy 2019-09-26 12:15:35 UTC
Comment on attachment 15493 [details]
patch for 4.10

LGTM
Comment 7 Andreas Schneider 2019-09-26 12:55:30 UTC
Karolin, could you please the patches to the corresponding branches? Thanks!
Comment 8 Karolin Seeger 2019-09-26 13:36:19 UTC
(In reply to Andreas Schneider from comment #7)
Pushed to autobuild-v4-{11,10}-test.
Comment 9 Stefan Metzmacher 2019-10-15 09:19:57 UTC
(In reply to Karolin Seeger from comment #8)

The 4.10 build failed with:

../../nsswitch/stress-nss-libwbclient.c:135: error: undefined reference to 'pthread_create'
../../nsswitch/stress-nss-libwbclient.c:142: error: undefined reference to 'pthread_create'
../../nsswitch/stress-nss-libwbclient.c:152: error: undefined reference to 'pthread_join'
collect2: error: ld returned 1 exit status

Build failed
 -> task in 'stress-nss-libwbclient' failed with exit status 1 (run with -v to display more information)
make: *** [all] Error 1
Comment 10 Isaac Boukris 2019-10-15 11:05:49 UTC
Created attachment 15542 [details]
wip patch for stress-nss-libwbclient

(In reply to Stefan Metzmacher from comment #9)

I think stress-nss-libwbclient should link against pthread, the reason we don't see it on master is because on master its wbclient dependency links against samba-util (libwbclient/wscript) so pthread gets pulled in anyway, but on that branch it hand't happen yet so it fails to build.

Attached patched works for me, pipeline:
https://gitlab.com/samba-team/devel/samba/tree/iboukris-pthread-stress-nss-libwbclient
Comment 11 Isaac Boukris 2019-10-15 12:18:10 UTC
Pipeline for 4.10 build:
https://gitlab.com/samba-team/devel/samba/pipelines/88930283
Comment 12 Stefan Metzmacher 2019-10-16 10:31:27 UTC
Comment on attachment 15542 [details]
wip patch for stress-nss-libwbclient

This looks change looks good.

Andreas, can you replace 'wip:' with 'nsswitch:',
add the bug reference and our review and push this to master?

I think we should revert ea7231dcc0b50c535f913f0542d600d0b2119a21
(libwbclient: Use wrapper for string to integer conversion)
from master and 4.11. libwbclient is supposed to have minimal dependencies.

The problem is bug tries to fix gets worse with ea7231dcc0b50c535f913f0542d600d0b2119a21.
Comment 13 Isaac Boukris 2019-10-16 11:01:53 UTC
(In reply to Stefan Metzmacher from comment #12)

@metze take a look at: https://gitlab.com/samba-team/samba/merge_requests/847

+1 for minimal deps in nss modules, we gave up nss_wins as it links to krb5 libs and pulls pthread from there anyway.
Comment 14 Andreas Schneider 2019-10-16 11:28:16 UTC
Created attachment 15551 [details]
additional patch for 4.11
Comment 15 Andreas Schneider 2019-10-16 11:30:07 UTC
Created attachment 15552 [details]
new patch for 4.10
Comment 16 Karolin Seeger 2019-10-16 19:25:51 UTC
(In reply to Andreas Schneider from comment #14)
Pushed to autobuild-v4-11-test.
Comment 17 Karolin Seeger 2019-10-16 19:26:48 UTC
(In reply to Andreas Schneider from comment #15)
Does not apply on current v4-10-test.
Comment 18 Karolin Seeger 2019-10-17 08:24:19 UTC
(In reply to Karolin Seeger from comment #17)
Pushed to v4-10-test.
Comment 19 Karolin Seeger 2019-10-17 09:08:04 UTC
Pushed to both branches.
Closing out bug report.

Thanks!
Comment 20 Andreas Schneider 2019-10-22 14:50:37 UTC
Created attachment 15561 [details]
additional patch for 4.10 to remove shmget and shm_open checks
Comment 21 Alexander Bokovoy 2019-10-22 14:57:43 UTC
Comment on attachment 15561 [details]
additional patch for 4.10 to remove shmget and shm_open checks

LGTM
Comment 22 Andreas Schneider 2019-10-22 15:10:57 UTC
Karolin, could you please apply the new patch to v4.10? Thanks!
Comment 23 Karolin Seeger 2019-10-30 10:03:02 UTC
(In reply to Andreas Schneider from comment #22)
Pushed additional test to autobuild-v4-10-test.
Comment 24 Karolin Seeger 2019-11-05 08:52:41 UTC
(In reply to Karolin Seeger from comment #23)
Pushed to v4-10-test.
Closing out bug report.

Thanks!