Ubuntu
sssd package

Merge ~lvoytek/ubuntu/+source/sssd:fix-client-fd-leak-kinetic into ubuntu/+source/sssd:ubuntu/kinetic-devel

  1. Git
  2. lp:~lvoytek/ubuntu/+source/sssd
  3. fix-client-fd-leak-kinetic
  4. Merge into ubuntu/kinetic-devel
Proposed by Lena Voytek
Status: Merged
Approved by: git-ubuntu bot
Approved revision: not available
Merge reported by: Lena Voytek
Merged at revision: 575d671ff8bdfbfe35cc1f18e4b7a95302d89821
Proposed branch: ~lvoytek/ubuntu/+source/sssd:fix-client-fd-leak-kinetic
Merge into: ubuntu/+source/sssd:ubuntu/kinetic-devel
Diff against target: 301 lines (+279/-0)
3 files modified
debian/changelog (+10/-0)
debian/patches/fix-client-fd-leak.patch (+268/-0)
debian/patches/series (+1/-0)
Reviewer Review Type Date Requested Status
git-ubuntu bot Approve
Athos Ribeiro (community) Approve
Canonical Server Reporter Pending
Review via email: mp+436860@code.launchpad.net

Description of the change

PPA: https://launchpad.net/~lvoytek/+archive/ubuntu/sssd-fix-client-fd-leak

autopkgtest results:
  sssd @ amd64:
    25.01.23 22:01:50 Log 🗒️ ✅ Triggers: sssd/2.7.3-2ubuntu3~ppa2
  sssd @ arm64:
    03.02.23 20:48:14 Log 🗒️ ✅ Triggers: sssd/2.7.3-2ubuntu3~ppa2
  sssd @ armhf:
    03.02.23 20:47:14 Log 🗒️ ✅ Triggers: sssd/2.7.3-2ubuntu3~ppa2
  sssd @ ppc64el:
    03.02.23 20:43:14 Log 🗒️ ✅ Triggers: sssd/2.7.3-2ubuntu3~ppa2
  sssd @ s390x:
    03.02.23 20:48:50 Log 🗒️ ✅ Triggers: sssd/2.7.3-2ubuntu3~ppa2

To post a comment you must log in.
Revision history for this message
Athos Ribeiro (athos-ribeiro) wrote :

Thanks, Lena!

LGTM. I did add a couple nitpick comments inline below.

review: Needs Information
~lvoytek/ubuntu/+source/sssd:fix-client-fd-leak-kinetic updated
575d671... by Lena Voytek

changelog

3180ab1... by Lena Voytek

* d/p/fix-client-fd-leak.patch:
  - close client socket at thread exit
  - only build lock-free client support if libc has required
    functionality for a proper cleanup
  - use proper mechanisms to init lock_mode only once

Revision history for this message
Lena Voytek (lvoytek) wrote :

Thanks for the review Athos! Updated based on your comments

Revision history for this message
Athos Ribeiro (athos-ribeiro) wrote :

LGTM! Thanks :)

review: Approve
Revision history for this message
git-ubuntu bot (git-ubuntu-bot) wrote :

Approvers: lvoytek, athos-ribeiro
Uploaders: lvoytek, athos-ribeiro
MP auto-approved

review: Approve
Revision history for this message
Lena Voytek (lvoytek) wrote :

Uploaded, thanks!
dput ubuntu ../sssd_2.7.3-2ubuntu2.1_source.changes
D: Setting host argument.
Checking signature on .changes
gpg: ../sssd_2.7.3-2ubuntu2.1_source.changes: Valid signature from 34B8AD7D9529E793
Checking signature on .dsc
gpg: ../sssd_2.7.3-2ubuntu2.1.dsc: Valid signature from 34B8AD7D9529E793
Uploading to ubuntu (via ftp to upload.ubuntu.com):
  Uploading sssd_2.7.3-2ubuntu2.1.dsc: done.
  Uploading sssd_2.7.3-2ubuntu2.1.debian.tar.xz: done.
  Uploading sssd_2.7.3-2ubuntu2.1_source.buildinfo: done.
  Uploading sssd_2.7.3-2ubuntu2.1_source.changes: done.
Successfully uploaded packages.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
diff --git a/debian/changelog b/debian/changelog
index e1685a4..45e547c 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,13 @@
1sssd (2.7.3-2ubuntu2.1) kinetic; urgency=medium
2
3 * d/p/fix-client-fd-leak.patch (LP: #1996869):
4 - close client socket at thread exit
5 - only build lock-free client support if libc has required
6 functionality for a proper cleanup
7 - use proper mechanisms to init lock_mode only once
8
9 -- Lena Voytek <lena.voytek@canonical.com> Fri, 20 Jan 2023 11:03:59 -0700
10
1sssd (2.7.3-2ubuntu2) kinetic; urgency=medium11sssd (2.7.3-2ubuntu2) kinetic; urgency=medium
212
3 * d/p/initialize-uid-gid-main-functions.patch: Initialize UID/GID13 * d/p/initialize-uid-gid-main-functions.patch: Initialize UID/GID
diff --git a/debian/patches/fix-client-fd-leak.patch b/debian/patches/fix-client-fd-leak.patch
4new file mode 10064414new file mode 100644
index 0000000..1521eee
--- /dev/null
+++ b/debian/patches/fix-client-fd-leak.patch
@@ -0,0 +1,268 @@
1Description: Fix client fd leak
2 Avoid leaking old client sockets by closing them properly on exit.
3 Also note that Lock-free client support will only be built if libc provides
4 `pthread_key_create()` and `pthread_once()`. This is relevant in glibc
5 version 2.34+.
6Author: Alexey Tikhonov <atikhono@redhat.com>
7Origin: upstream, https://github.com/SSSD/sssd/commit/1a6f67c92399ff8e358a6c6cdda43fb2547a5fdb
8Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/sssd/+bug/1996869
9Last-Update: 2023-01-25
10---
11This patch header follows DEP-3: http://dep.debian.net/deps/dep3/
12--- a/configure.ac
13+++ b/configure.ac
14@@ -51,18 +51,39 @@
15 m4_include([src/build_macros.m4])
16 BUILD_WITH_SHARED_BUILD_DIR
17
18-AC_COMPILE_IFELSE(
19+
20+SAVE_LIBS=$LIBS
21+LIBS=
22+AC_LINK_IFELSE(
23 [AC_LANG_PROGRAM([[#include <pthread.h>]],
24 [[pthread_mutex_t m = PTHREAD_MUTEX_INITIALIZER;
25- (void) m; /* unused */
26+ pthread_mutex_lock(&m);
27+ pthread_mutex_unlock(&m);
28 ]])],
29 [AC_DEFINE([HAVE_PTHREAD], [1], [Pthread mutexes available.])
30 HAVE_PTHREAD=1
31 ],
32- [AC_MSG_WARN([Pthread library not found! Clients will not be thread safe...])])
33+ [AC_MSG_WARN([Pthread mutex support not found! Clients will not be thread safe...])])
34+LIBS=$SAVE_LIBS
35+AM_CONDITIONAL([HAVE_PTHREAD], [test x"$HAVE_PTHREAD" != "x"])
36
37
38-AM_CONDITIONAL([HAVE_PTHREAD], [test x"$HAVE_PTHREAD" != "x"])
39+SAVE_LIBS=$LIBS
40+LIBS=
41+AC_LINK_IFELSE(
42+ [AC_LANG_PROGRAM([[#include <pthread.h>]],
43+ [[static pthread_key_t k;
44+ static pthread_once_t f = PTHREAD_ONCE_INIT;
45+ pthread_once(&f, NULL);
46+ pthread_key_create(&k, NULL);
47+ ]])],
48+ [AC_DEFINE([HAVE_PTHREAD_EXT], [1], [Extended pthread functionality is available.])
49+ HAVE_PTHREAD_EXT=1
50+ ],
51+ [AC_MSG_WARN([Extended pthread functionality is not available. Lock-free client feature will not be built.])])
52+LIBS=$SAVE_LIBS
53+AM_CONDITIONAL([BUILD_LOCKFREE_CLIENT], [test x"$HAVE_PTHREAD_EXT" != "x"])
54+
55
56 # Check library for the timer_create function
57 SAVE_LIBS=$LIBS
58--- a/src/man/Makefile.am
59+++ b/src/man/Makefile.am
60@@ -46,9 +46,12 @@
61 if BUILD_KCM_RENEWAL
62 KCM_RENEWAL_CONDS = ;enable_kcm_renewal
63 endif
64+if BUILD_LOCKFREE_CLIENT
65+LOCKFREE_CLIENT_CONDS = ;enable_lockfree_support
66+endif
67
68
69-CONDS = with_false$(SUDO_CONDS)$(AUTOFS_CONDS)$(SSH_CONDS)$(PAC_RESPONDER_CONDS)$(IFP_CONDS)$(GPO_CONDS)$(SYSTEMD_CONDS)$(FILES_CONDS)$(KCM_CONDS)$(STAP_CONDS)$(KCM_RENEWAL_CONDS)
70+CONDS = with_false$(SUDO_CONDS)$(AUTOFS_CONDS)$(SSH_CONDS)$(PAC_RESPONDER_CONDS)$(IFP_CONDS)$(GPO_CONDS)$(SYSTEMD_CONDS)$(FILES_CONDS)$(KCM_CONDS)$(STAP_CONDS)$(KCM_RENEWAL_CONDS)$(LOCKFREE_CLIENT_CONDS)
71
72
73 #Special Rules:
74--- a/src/man/sssd.8.xml
75+++ b/src/man/sssd.8.xml
76@@ -240,7 +240,7 @@
77 If the environment variable SSS_NSS_USE_MEMCACHE is set to "NO",
78 client applications will not use the fast in-memory cache.
79 </para>
80- <para>
81+ <para condition="enable_lockfree_support">
82 If the environment variable SSS_LOCKFREE is set to "NO", requests
83 from multiple threads of a single application will be serialized.
84 </para>
85--- a/src/sss_client/common.c
86+++ b/src/sss_client/common.c
87@@ -35,7 +35,6 @@
88 #include <stdlib.h>
89 #include <stdbool.h>
90 #include <stdint.h>
91-#include <stdatomic.h>
92 #include <string.h>
93 #include <fcntl.h>
94 #include <poll.h>
95@@ -62,8 +61,15 @@
96
97 /* common functions */
98
99+#ifdef HAVE_PTHREAD_EXT
100+static pthread_key_t sss_sd_key;
101+static pthread_once_t sss_sd_key_initialized = PTHREAD_ONCE_INIT;
102 static __thread int sss_cli_sd = -1; /* the sss client socket descriptor */
103 static __thread struct stat sss_cli_sb; /* the sss client stat buffer */
104+#else
105+static int sss_cli_sd = -1; /* the sss client socket descriptor */
106+static struct stat sss_cli_sb; /* the sss client stat buffer */
107+#endif
108
109 #if HAVE_FUNCTION_ATTRIBUTE_DESTRUCTOR
110 __attribute__((destructor))
111@@ -76,6 +82,18 @@
112 }
113 }
114
115+#ifdef HAVE_PTHREAD_EXT
116+static void sss_at_thread_exit(void *v)
117+{
118+ sss_cli_close_socket();
119+}
120+
121+static void init_sd_key(void)
122+{
123+ pthread_key_create(&sss_sd_key, sss_at_thread_exit);
124+}
125+#endif
126+
127 /* Requests:
128 *
129 * byte 0-3: 32bit unsigned with length (the complete packet length: 0 to X)
130@@ -553,6 +571,16 @@
131 return -1;
132 }
133
134+#ifdef HAVE_PTHREAD_EXT
135+ pthread_once(&sss_sd_key_initialized, init_sd_key); /* once for all threads */
136+
137+ /* It actually doesn't matter what value to set for a key.
138+ * The only important thing: key must be non-NULL to ensure
139+ * destructor is executed at thread exit.
140+ */
141+ pthread_setspecific(sss_sd_key, &sss_cli_sd);
142+#endif
143+
144 /* set as non-blocking, close on exec, and make sure standard
145 * descriptors are not used */
146 sd = make_safe_fd(sd);
147@@ -1129,41 +1157,38 @@
148 }
149
150 #if HAVE_PTHREAD
151-bool sss_is_lockfree_mode(void)
152+
153+#ifdef HAVE_PTHREAD_EXT
154+static bool sss_lock_free = true;
155+static pthread_once_t sss_lock_mode_initialized = PTHREAD_ONCE_INIT;
156+
157+static void init_lock_mode(void)
158 {
159- const char *env = NULL;
160- enum {
161- MODE_UNDEF,
162- MODE_LOCKING,
163- MODE_LOCKFREE
164- };
165- static atomic_int mode = MODE_UNDEF;
166-
167- if (mode == MODE_UNDEF) {
168- env = getenv("SSS_LOCKFREE");
169- if ((env != NULL) && (strcasecmp(env, "NO") == 0)) {
170- mode = MODE_LOCKING;
171- } else {
172- mode = MODE_LOCKFREE;
173- }
174+ const char *env = getenv("SSS_LOCKFREE");
175+
176+ if ((env != NULL) && (strcasecmp(env, "NO") == 0)) {
177+ sss_lock_free = false;
178 }
179+}
180
181- return (mode == MODE_LOCKFREE);
182+bool sss_is_lockfree_mode(void)
183+{
184+ pthread_once(&sss_lock_mode_initialized, init_lock_mode);
185+ return sss_lock_free;
186 }
187+#endif
188
189 struct sss_mutex sss_nss_mtx = { .mtx = PTHREAD_MUTEX_INITIALIZER };
190-
191 static struct sss_mutex sss_pam_mtx = { .mtx = PTHREAD_MUTEX_INITIALIZER };
192-
193-static struct sss_mutex sss_nss_mc_mtx = { .mtx = PTHREAD_MUTEX_INITIALIZER };
194-
195 static struct sss_mutex sss_pac_mtx = { .mtx = PTHREAD_MUTEX_INITIALIZER };
196
197 static void sss_mt_lock(struct sss_mutex *m)
198 {
199+#ifdef HAVE_PTHREAD_EXT
200 if (sss_is_lockfree_mode()) {
201 return;
202 }
203+#endif
204
205 pthread_mutex_lock(&m->mtx);
206 pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &m->old_cancel_state);
207@@ -1171,9 +1196,11 @@
208
209 static void sss_mt_unlock(struct sss_mutex *m)
210 {
211+#ifdef HAVE_PTHREAD_EXT
212 if (sss_is_lockfree_mode()) {
213 return;
214 }
215+#endif
216
217 pthread_setcancelstate(m->old_cancel_state, NULL);
218 pthread_mutex_unlock(&m->mtx);
219@@ -1189,7 +1216,7 @@
220 sss_mt_unlock(&sss_nss_mtx);
221 }
222
223-/* NSS mutex wrappers */
224+/* PAM mutex wrappers */
225 void sss_pam_lock(void)
226 {
227 sss_mt_lock(&sss_pam_mtx);
228@@ -1199,16 +1226,6 @@
229 sss_mt_unlock(&sss_pam_mtx);
230 }
231
232-/* NSS mutex wrappers */
233-void sss_nss_mc_lock(void)
234-{
235- sss_mt_lock(&sss_nss_mc_mtx);
236-}
237-void sss_nss_mc_unlock(void)
238-{
239- sss_mt_unlock(&sss_nss_mc_mtx);
240-}
241-
242 /* PAC mutex wrappers */
243 void sss_pac_lock(void)
244 {
245--- a/src/sss_client/idmap/common_ex.c
246+++ b/src/sss_client/idmap/common_ex.c
247@@ -28,7 +28,9 @@
248 #include "common_private.h"
249
250 extern struct sss_mutex sss_nss_mtx;
251+#ifdef HAVE_PTHREAD_EXT
252 bool sss_is_lockfree_mode(void);
253+#endif
254
255 #define SEC_FROM_MSEC(ms) ((ms) / 1000)
256 #define NSEC_FROM_MSEC(ms) (((ms) % 1000) * 1000 * 1000)
257@@ -51,9 +53,11 @@
258 {
259 int ret;
260
261+#ifdef HAVE_PTHREAD_EXT
262 if (sss_is_lockfree_mode()) {
263 return 0;
264 }
265+#endif
266
267 ret = pthread_mutex_timedlock(&m->mtx, endtime);
268 if (ret != 0) {
diff --git a/debian/patches/series b/debian/patches/series
index b5cc56b..f47c99f 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -3,3 +3,4 @@ default-to-socket-activated-services.diff
3fix-shebang-on-sss_analyze.patch3fix-shebang-on-sss_analyze.patch
4support-krb5-1.20.diff4support-krb5-1.20.diff
5initialize-uid-gid-main-functions.patch5initialize-uid-gid-main-functions.patch
6fix-client-fd-leak.patch

Subscribers

People subscribed via source and target branches