This patchset implements a new responder like service in SSSD called secrets. It uses the Custodia project API to offer a service where applications/users can store secrets in a way that makes requests remotizable and routable with a high degree of configurability (esp, in conjunction with a Custodia proxy).
Included are also accessory patches to change the monitor and other aspects of service startup and monitoring necessary to have this new kind of service which is more independent than the pam/nss based services.
There is no testsuite for the service yet.
The work is also not complete in that the monitor does not start the service yet, I have an experimental unit file I am working on but it is not fully functional and not included yet..
I do not expect all patches to be accepted right away, but they all work individually (manually tested), but I think it is a good time to start review and bring in what works, as we are going to spread some of the remaining work across multiple people.
HTH, Simo.
On (30/03/16 12:31), Simo Sorce wrote:
This patchset implements a new responder like service in SSSD called secrets. It uses the Custodia project API to offer a service where applications/users can store secrets in a way that makes requests remotizable and routable with a high degree of configurability (esp, in conjunction with a Custodia proxy).
Included are also accessory patches to change the monitor and other aspects of service startup and monitoring necessary to have this new kind of service which is more independent than the pam/nss based services.
There is no testsuite for the service yet.
The work is also not complete in that the monitor does not start the service yet, I have an experimental unit file I am working on but it is not fully functional and not included yet..
I do not expect all patches to be accepted right away, but they all work individually (manually tested), but I think it is a good time to start review and bring in what works, as we are going to spread some of the remaining work across multiple people.
HTH, Simo.
I do not plan to do a full review. Jakub voluntered IIRC :-) But here are few comments.
From cd731590f1830ab9686949af1fa66d2b7463eafc Mon Sep 17 00:00:00 2001 From: Simo Sorce simo@redhat.com Date: Tue, 12 Jan 2016 20:07:59 -0500 Subject: [PATCH 01/15] Util: Add watchdog helper
The watchdog uses a kernel timer to issue a signal to the process. It checks if the ticker is not being reset by the main event loop, which would indicate that the process got stuck. At the same time it sets a tevent timer to clear the watchdog ticker, so that the watchdog handler is kept happy.
If the watchdog detects that the timer event failed to reset the watchdog for three times in a row then the process is killed. Normally the monitor will detect the child terminated and will rescheduled it.
Related: https://fedorahosted.org/sssd/ticket/2921
diff --git a/src/util/util_watchdog.c b/src/util/util_watchdog.c new file mode 100644 index 0000000000000000000000000000000000000000..64faaaf03213319d3127fde3946124ee26c7794a --- /dev/null +++ b/src/util/util_watchdog.c
//snip
+int setup_watchdog(struct tevent_context *ev, int interval) +{
- struct sigevent sev;
- struct itimerspec its;
- int signum = SIGRTMIN;
- int ret;
- CatchSignal(signum, watchdog_handler);
- sev.sigev_notify = SIGEV_SIGNAL;
- sev.sigev_signo = signum;
- sev.sigev_value.sival_ptr = &watchdog_ctx.timerid;
- errno = 0;
- ret = timer_create(CLOCK_MONOTONIC, &sev, &watchdog_ctx.timerid);
I got a valgrind error here on rhel6 ==2376== Syscall param timer_create(evp) points to uninitialised byte(s) ==2376== at 0x5B0B08D: timer_create@@GLIBC_2.3.3 (in /lib64/librt-2.12.so) ==2376== by 0x54942CD: setup_watchdog (util_watchdog.c:88) ==2376== by 0x40414F: server_setup (server.c:651) ==2376== by 0x403059: test_run_as_root_fg (test_server.c:104) ==2376== by 0x5243A94: ??? (in /usr/lib64/libcmocka.so.0.3.1) ==2376== by 0x5243DA9: _cmocka_run_group_tests (in /usr/lib64/libcmocka.so.0.3.1) ==2376== by 0x402CA1: main (test_server.c:204) ==2376== Address 0x7fefff920 is on thread 1's stack
Following diff fixed it.
diff --git a/src/util/util_watchdog.c b/src/util/util_watchdog.c index 64faaaf..d6f71ad 100644 --- a/src/util/util_watchdog.c +++ b/src/util/util_watchdog.c @@ -74,7 +74,7 @@ static void watchdog_event_handler(struct tevent_context *ev,
int setup_watchdog(struct tevent_context *ev, int interval) { - struct sigevent sev; + struct sigevent sev = { 0, }; struct itimerspec its; int signum = SIGRTMIN; int ret;
- if (ret == -1) {
ret = errno;DEBUG(SSSDBG_FATAL_FAILURE,"Failed to create watchdog timer (%d) [%s]\n",ret, strerror(ret));return ret;- }
- if (interval == 0) {
interval = WATCHDOG_DEF_INTERVAL;- }
- watchdog_ctx.interval.tv_sec = interval;
- watchdog_ctx.interval.tv_usec = 0;
- /* Start the timer */
- /* we give 1 second head start to the watchdog event */
- its.it_value.tv_sec = interval + 1;
- its.it_value.tv_nsec = 0;
- its.it_interval.tv_sec = interval;
- its.it_interval.tv_nsec = 0;
- errno = 0;
- ret = timer_settime(watchdog_ctx.timerid, 0, &its, NULL);
- if (ret == -1) {
ret = errno;DEBUG(SSSDBG_FATAL_FAILURE,"Failed to create watchdog timer (%d) [%s]\n",ret, strerror(ret));return ret;- }
- /* Add the watchdog event and make it fire as fast as the timer */
- watchdog_event_handler(ev, NULL, tevent_timeval_zero(), NULL);
- return EOK;
+}
+void teardown_watchdog(void) +{
- int ret;
- /* Disarm the timer */
- errno = 0;
- ret = timer_delete(watchdog_ctx.timerid);
- if (ret == -1) {
ret = errno;DEBUG(SSSDBG_FATAL_FAILURE,"Failed to destroy watchdog timer (%d) [%s]\n",ret, strerror(ret));- }
- /* and kill the watchdog event */
- talloc_free(watchdog_ctx.te);
+}
-- 2.5.5
From 33c3b8ad97f37ca06f00eaf6be747360e9f4947b Mon Sep 17 00:00:00 2001 From: Simo Sorce simo@redhat.com Date: Fri, 8 Jan 2016 10:56:17 -0500 Subject: [PATCH 07/15] Secrets: Add autoconf macros to build with secrets
Prepares autoconf for the new Secrets Provider
Related: https://fedorahosted.org/sssd/ticket/2913
configure.ac | 3 +++ src/conf_macros.m4 | 43 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+)
diff --git a/configure.ac b/configure.ac index 2c36049ca04f57fe94f359c5dc19a506dd2b9388..783372dd6b5d86793c218ac513a93b65e97d4c06 100644 --- a/configure.ac +++ b/configure.ac @@ -145,6 +145,9 @@ WITH_NFS WITH_NFS_LIB_PATH WITH_LIBWBCLIENT WITH_SSSD_USER +SSSD_RUNSTATEDIR +WITH_SECRETS +WITH_SECRETS_DB_PATH
m4_include([src/external/pkg.m4]) m4_include([src/external/libpopt.m4]) diff --git a/src/conf_macros.m4 b/src/conf_macros.m4 index c8774b5f5b7d1be09ee74195bc0732984dd551d7..e0a64470da4ac0c885274bfbb066b35873e0911b 100644 --- a/src/conf_macros.m4 +++ b/src/conf_macros.m4 @@ -847,3 +847,46 @@ AC_DEFUN([ENABLE_POLKIT_RULES_PATH],
AM_CONDITIONAL([HAVE_POLKIT_RULES_D], [test x$HAVE_POLKIT_RULES_D != x])])
+dnl BAckwards compat for older autoconf +AC_DEFUN([SSSD_RUNSTATEDIR],
- [
- if test x"$runstatedir" = x; then
AC_SUBST([runstatedir],["${localstatedir}/run"])- fi
- ])
+AC_DEFUN([WITH_SECRETS],
- [ AC_ARG_WITH([secrets],
[AC_HELP_STRING([--with-secrets],[Whether to build with secrets support [yes]])],[with_secrets=$withval],with_secrets=yes)- if test x"$with_secrets" = xyes; then
AC_DEFINE(BUILD_SECRETS, 1, [whether to build with SECRETS support])- fi
- AM_CONDITIONAL([BUILD_SECRETS], [test x"$with_secrets" = xyes])
- ])
+AC_DEFUN([WITH_SECRETS_DB_PATH],
- [ AC_ARG_WITH([secrets-db-path],
[AC_HELP_STRING([--with-secrets-db-path=PATH],[Path to the SSSD databases [/var/lib/sss/secrets]])])- config_secdbpath=""SSS_STATEDIR"/secrets"
- secdbpath="${localstatedir}/lib/sss/secrets"
- if test x"$with_secrets_db_path" != x; then
config_secdbpath=$with_secrets_db_pathsecdbpath=$with_secrets_db_path- fi
- AC_SUBST(secdbpath)
- AC_DEFINE_UNQUOTED(SECRETS_DB_PATH, "$config_secdbpath", [Path to the SSSD Secrets databases])
- ])
-- 2.5.5
From deafb4c8a72a4500353b03765fd97406759ee4e0 Mon Sep 17 00:00:00 2001 From: Christian Heimes cheimes@redhat.com Date: Fri, 8 Jan 2016 13:26:22 +0100 Subject: [PATCH 08/15] Secrets: m4 macros for jansson and http-parser
Prepares autoconf for the new Secrets Provider dependencies
Related: https://fedorahosted.org/sssd/ticket/2913
Signed-off-by: Christian Heimes cheimes@redhat.com Reviewd-by: Simo Sorce simo@redhat.com
configure.ac | 2 ++ src/external/libhttp_parser.m4 | 15 +++++++++++++++ src/external/libjansson.m4 | 15 +++++++++++++++ 3 files changed, 32 insertions(+) create mode 100644 src/external/libhttp_parser.m4 create mode 100644 src/external/libjansson.m4
diff --git a/configure.ac b/configure.ac index 783372dd6b5d86793c218ac513a93b65e97d4c06..f7430ca71f07b1085f49a7635070f71894f1a4a9 100644 --- a/configure.ac +++ b/configure.ac @@ -185,6 +185,8 @@ m4_include([src/external/libnfsidmap.m4]) m4_include([src/external/cwrap.m4]) m4_include([src/external/libresolv.m4]) m4_include([src/external/intgcheck.m4]) +m4_include([src/external/libhttp_parser.m4]) +m4_include([src/external/libjansson.m4])
This patch broke build in mock and CI. because you added new dependencies which are not as BuildRequires in spec. and moreover these dependencies are strictly detected at configure time even though secrect service can be build optionally.
One solution would be to remove conitional build of secret service or another to fix detection of build dependencies at configure time.
e.g. diff --git a/configure.ac b/configure.ac index 2a8329c..9ddd9da 100644 --- a/configure.ac +++ b/configure.ac @@ -186,8 +186,11 @@ m4_include([src/external/libnfsidmap.m4]) m4_include([src/external/cwrap.m4]) m4_include([src/external/libresolv.m4]) m4_include([src/external/intgcheck.m4]) -m4_include([src/external/libhttp_parser.m4]) -m4_include([src/external/libjansson.m4]) + +if test x$with_secrets = xyes; then + m4_include([src/external/libhttp_parser.m4]) + m4_include([src/external/libjansson.m4]) +fi
if test x$build_config_lib = xyes; then m4_include([src/external/libaugeas.m4])
or you can move if condition directly to m4_included files.
And here is a diff for adding missing build dependencies diff --git a/contrib/ci/deps.sh b/contrib/ci/deps.sh index c9a8a63..8120347 100644 --- a/contrib/ci/deps.sh +++ b/contrib/ci/deps.sh @@ -114,6 +114,8 @@ if [[ "$DISTRO_BRANCH" == -debian-* ]]; then python-ldap ldap-utils slapd + libhttp-parser-dev + libjansson-dev ) DEPS_INTGCHECK_SATISFIED=true fi diff --git a/contrib/sssd.spec.in b/contrib/sssd.spec.in index 979f390..c0709ea 100644 --- a/contrib/sssd.spec.in +++ b/contrib/sssd.spec.in @@ -162,6 +162,9 @@ BuildRequires: nfs-utils-lib-devel BuildRequires: samba4-devel BuildRequires: libsmbclient-devel
+BuildRequires: http-parser-devel +BuildRequires: jansson-devel + %description Provides a set of daemons to manage access to remote directories and authentication mechanisms. It provides an NSS and PAM interface toward
There are also some warnings reported by static analyzers. Error: PW.MIXED_ENUM_TYPE: [#def4] sssd-1.13.90/src/responder/secrets/providers.c:318: mixed_enum_type: enumerated type mixed with another type # 316| req->length = 0; # 317| # 318|-> cmd = http_method_str(method); # 319| if (cmd == NULL) return EINVAL; # 320|
Error: PW.MIXED_ENUM_TYPE: [#def5] sssd-1.13.90/src/responder/secrets/proxy.c:249: mixed_enum_type: enumerated type mixed with another type # 247| /* Request-Line */ # 248| req->data = talloc_asprintf(req, "%s %s HTTP/1.1\r\n", # 249|-> http_method_str(secreq->method), http_uri); # 250| if (!req->data) { # 251| ret = ENOMEM;
Error: UNUSED_VALUE (CWE-563): [#def9] sssd-1.13.90/src/util/server.c:651: value_overwrite: Overwriting previous write to "ret" with value from "setup_watchdog(ctx->event_ctx, watchdog_interval)". sssd-1.13.90/src/util/server.c:648: returned_value: Assigning value from "confdb_get_int(ctx->confdb_ctx, conf_entry, "timeout", 0, &watchdog_interval)" to "ret" here, but that stored value is overwritten before it can be used. # 646| # 647| /* Setup the internal watchdog */ # 648|-> ret = confdb_get_int(ctx->confdb_ctx, conf_entry, # 649| CONFDB_DOMAIN_TIMEOUT, # 650| 0, &watchdog_interval);
Following two seems to be the same. One is from coverity and second from clang Error: UNINIT (CWE-457): [#def7] sssd-1.13.90/src/responder/secrets/proxy.c:925: var_decl: Declaring variable "reply" without initializer. sssd-1.13.90/src/responder/secrets/proxy.c:939: uninit_use: Using uninitialized value "reply". # 937| } # 938| # 939|-> ret = sec_http_reply_with_headers(state->secreq, &state->secreq->reply, # 940| reply->status_code, reply->reason_phrase, # 941| reply->headers, reply->num_headers,
Error: CLANG_WARNING: [#def8] sssd-1.13.90/src/responder/secrets/proxy.c:940:39: warning: Dereference of undefined pointer value # reply->status_code, reply->reason_phrase, # ^~~~~~~~~~~~~~~~~~ sssd-1.13.90/src/responder/secrets/proxy.c:925:5: note: 'reply' declared without an initial value # struct proxy_http_reply *reply; # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ sssd-1.13.90/src/responder/secrets/proxy.c:931:11: note: Calling 'proxy_http_req_recv' # ret = proxy_http_req_recv(subreq, state, &reply); # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ sssd-1.13.90/src/responder/secrets/proxy.c:550:5: note: Taking true branch # TEVENT_REQ_RETURN_ON_ERROR(req); # ^ sssd-1.13.90/src/util/util.h:104:5: note: expanded from macro 'TEVENT_REQ_RETURN_ON_ERROR' # if (tevent_req_is_error(req, &TRROEstate, &TRROEerr)) { \ # ^ sssd-1.13.90/src/responder/secrets/proxy.c:550:5: note: Assuming 'TRROEstate' is equal to TEVENT_REQ_USER_ERROR # TEVENT_REQ_RETURN_ON_ERROR(req); # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ sssd-1.13.90/src/util/util.h:105:13: note: expanded from macro 'TEVENT_REQ_RETURN_ON_ERROR' # if (TRROEstate == TEVENT_REQ_USER_ERROR) { \ # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ sssd-1.13.90/src/responder/secrets/proxy.c:550:5: note: Taking true branch sssd-1.13.90/src/util/util.h:105:9: note: expanded from macro 'TEVENT_REQ_RETURN_ON_ERROR' # if (TRROEstate == TEVENT_REQ_USER_ERROR) { \ # ^ sssd-1.13.90/src/responder/secrets/proxy.c:931:11: note: Returning from 'proxy_http_req_recv' # ret = proxy_http_req_recv(subreq, state, &reply); # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ sssd-1.13.90/src/responder/secrets/proxy.c:934:9: note: Assuming 'ret' is equal to 0 # if (ret != EOK) { # ^~~~~~~~~~ sssd-1.13.90/src/responder/secrets/proxy.c:934:5: note: Taking false branch # if (ret != EOK) { # ^ sssd-1.13.90/src/responder/secrets/proxy.c:940:39: note: Dereference of undefined pointer value # reply->status_code, reply->reason_phrase, # ^~~~~~~~~~~~~~~~~~ # 938| # 939| ret = sec_http_reply_with_headers(state->secreq, &state->secreq->reply, # 940|-> reply->status_code, reply->reason_phrase, # 941| reply->headers, reply->num_headers, # 942| &reply->body);
LS
On Fri, 2016-04-01 at 13:05 +0200, Lukas Slebodnik wrote:
On (30/03/16 12:31), Simo Sorce wrote:
This patchset implements a new responder like service in SSSD called secrets. It uses the Custodia project API to offer a service where applications/users can store secrets in a way that makes requests remotizable and routable with a high degree of configurability (esp, in conjunction with a Custodia proxy).
Included are also accessory patches to change the monitor and other aspects of service startup and monitoring necessary to have this new kind of service which is more independent than the pam/nss based services.
There is no testsuite for the service yet.
The work is also not complete in that the monitor does not start the service yet, I have an experimental unit file I am working on but it is not fully functional and not included yet..
I do not expect all patches to be accepted right away, but they all work individually (manually tested), but I think it is a good time to start review and bring in what works, as we are going to spread some of the remaining work across multiple people.
HTH, Simo.
I do not plan to do a full review. Jakub voluntered IIRC :-) But here are few comments.
From cd731590f1830ab9686949af1fa66d2b7463eafc Mon Sep 17 00:00:00 2001 From: Simo Sorce simo@redhat.com Date: Tue, 12 Jan 2016 20:07:59 -0500 Subject: [PATCH 01/15] Util: Add watchdog helper
The watchdog uses a kernel timer to issue a signal to the process. It checks if the ticker is not being reset by the main event loop, which would indicate that the process got stuck. At the same time it sets a tevent timer to clear the watchdog ticker, so that the watchdog handler is kept happy.
If the watchdog detects that the timer event failed to reset the watchdog for three times in a row then the process is killed. Normally the monitor will detect the child terminated and will rescheduled it.
Related: https://fedorahosted.org/sssd/ticket/2921
diff --git a/src/util/util_watchdog.c b/src/util/util_watchdog.c new file mode 100644 index 0000000000000000000000000000000000000000..64faaaf03213319d3127fde3946124ee26c7794a --- /dev/null +++ b/src/util/util_watchdog.c
//snip
+int setup_watchdog(struct tevent_context *ev, int interval) +{
- struct sigevent sev;
- struct itimerspec its;
- int signum = SIGRTMIN;
- int ret;
- CatchSignal(signum, watchdog_handler);
- sev.sigev_notify = SIGEV_SIGNAL;
- sev.sigev_signo = signum;
- sev.sigev_value.sival_ptr = &watchdog_ctx.timerid;
- errno = 0;
- ret = timer_create(CLOCK_MONOTONIC, &sev, &watchdog_ctx.timerid);
I got a valgrind error here on rhel6 ==2376== Syscall param timer_create(evp) points to uninitialised byte(s) ==2376== at 0x5B0B08D: timer_create@@GLIBC_2.3.3 (in /lib64/librt-2.12.so) ==2376== by 0x54942CD: setup_watchdog (util_watchdog.c:88) ==2376== by 0x40414F: server_setup (server.c:651) ==2376== by 0x403059: test_run_as_root_fg (test_server.c:104) ==2376== by 0x5243A94: ??? (in /usr/lib64/libcmocka.so.0.3.1) ==2376== by 0x5243DA9: _cmocka_run_group_tests (in /usr/lib64/libcmocka.so.0.3.1) ==2376== by 0x402CA1: main (test_server.c:204) ==2376== Address 0x7fefff920 is on thread 1's stack
Following diff fixed it.
Thanks for finding this!
diff --git a/src/util/util_watchdog.c b/src/util/util_watchdog.c index 64faaaf..d6f71ad 100644 --- a/src/util/util_watchdog.c +++ b/src/util/util_watchdog.c @@ -74,7 +74,7 @@ static void watchdog_event_handler(struct tevent_context *ev,
int setup_watchdog(struct tevent_context *ev, int interval) {
- struct sigevent sev;
- struct sigevent sev = { 0, }; struct itimerspec its; int signum = SIGRTMIN; int ret;
diff --git a/configure.ac b/configure.ac index 783372dd6b5d86793c218ac513a93b65e97d4c06..f7430ca71f07b1085f49a7635070f71894f1a4a9 100644 --- a/configure.ac +++ b/configure.ac @@ -185,6 +185,8 @@ m4_include([src/external/libnfsidmap.m4]) m4_include([src/external/cwrap.m4]) m4_include([src/external/libresolv.m4]) m4_include([src/external/intgcheck.m4]) +m4_include([src/external/libhttp_parser.m4]) +m4_include([src/external/libjansson.m4])
This patch broke build in mock and CI. because you added new dependencies which are not as BuildRequires in spec. and moreover these dependencies are strictly detected at configure time even though secrect service can be build optionally.
One solution would be to remove conitional build of secret service or another to fix detection of build dependencies at configure time.
I'll fix the detection.
e.g. diff --git a/configure.ac b/configure.ac index 2a8329c..9ddd9da 100644 --- a/configure.ac +++ b/configure.ac @@ -186,8 +186,11 @@ m4_include([src/external/libnfsidmap.m4]) m4_include([src/external/cwrap.m4]) m4_include([src/external/libresolv.m4]) m4_include([src/external/intgcheck.m4]) -m4_include([src/external/libhttp_parser.m4]) -m4_include([src/external/libjansson.m4])
+if test x$with_secrets = xyes; then
- m4_include([src/external/libhttp_parser.m4])
- m4_include([src/external/libjansson.m4])
+fi
if test x$build_config_lib = xyes; then m4_include([src/external/libaugeas.m4])
or you can move if condition directly to m4_included files.
The above looks ok to me.
And here is a diff for adding missing build dependencies diff --git a/contrib/ci/deps.sh b/contrib/ci/deps.sh index c9a8a63..8120347 100644 --- a/contrib/ci/deps.sh +++ b/contrib/ci/deps.sh @@ -114,6 +114,8 @@ if [[ "$DISTRO_BRANCH" == -debian-* ]]; then python-ldap ldap-utils slapd
libhttp-parser-devlibjansson-devfi diff --git a/contrib/sssd.spec.in b/contrib/sssd.spec.in index 979f390..c0709ea 100644 --- a/contrib/sssd.spec.in +++ b/contrib/sssd.spec.in @@ -162,6 +162,9 @@ BuildRequires: nfs-utils-lib-devel BuildRequires: samba4-devel BuildRequires: libsmbclient-devel
+BuildRequires: http-parser-devel +BuildRequires: jansson-devel
%description Provides a set of daemons to manage access to remote directories and authentication mechanisms. It provides an NSS and PAM interface toward
Thanks!
There are also some warnings reported by static analyzers. Error: PW.MIXED_ENUM_TYPE: [#def4] sssd-1.13.90/src/responder/secrets/providers.c:318: mixed_enum_type: enumerated type mixed with another type # 316| req->length = 0; # 317| # 318|-> cmd = http_method_str(method); # 319| if (cmd == NULL) return EINVAL; # 320|
Error: PW.MIXED_ENUM_TYPE: [#def5] sssd-1.13.90/src/responder/secrets/proxy.c:249: mixed_enum_type: enumerated type mixed with another type # 247| /* Request-Line */ # 248| req->data = talloc_asprintf(req, "%s %s HTTP/1.1\r\n", # 249|-> http_method_str(secreq->method), http_uri); # 250| if (!req->data) { # 251| ret = ENOMEM; Error: UNUSED_VALUE (CWE-563): [#def9] sssd-1.13.90/src/util/server.c:651: value_overwrite: Overwriting previous write to "ret" with value from "setup_watchdog(ctx->event_ctx, watchdog_interval)". sssd-1.13.90/src/util/server.c:648: returned_value: Assigning value from "confdb_get_int(ctx->confdb_ctx, conf_entry, "timeout", 0, &watchdog_interval)" to "ret" here, but that stored value is overwritten before it can be used. # 646| # 647| /* Setup the internal watchdog */ # 648|-> ret = confdb_get_int(ctx->confdb_ctx, conf_entry, # 649| CONFDB_DOMAIN_TIMEOUT, # 650| 0, &watchdog_interval);Following two seems to be the same. One is from coverity and second from clang Error: UNINIT (CWE-457): [#def7] sssd-1.13.90/src/responder/secrets/proxy.c:925: var_decl: Declaring variable "reply" without initializer. sssd-1.13.90/src/responder/secrets/proxy.c:939: uninit_use: Using uninitialized value "reply". # 937| } # 938| # 939|-> ret = sec_http_reply_with_headers(state->secreq, &state->secreq->reply, # 940| reply->status_code, reply->reason_phrase, # 941| reply->headers, reply->num_headers,
Error: CLANG_WARNING: [#def8] sssd-1.13.90/src/responder/secrets/proxy.c:940:39: warning: Dereference of undefined pointer value # reply->status_code, reply->reason_phrase, # ^~~~~~~~~~~~~~~~~~ sssd-1.13.90/src/responder/secrets/proxy.c:925:5: note: 'reply' declared without an initial value # struct proxy_http_reply *reply; # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ sssd-1.13.90/src/responder/secrets/proxy.c:931:11: note: Calling 'proxy_http_req_recv' # ret = proxy_http_req_recv(subreq, state, &reply); # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ sssd-1.13.90/src/responder/secrets/proxy.c:550:5: note: Taking true branch # TEVENT_REQ_RETURN_ON_ERROR(req); # ^ sssd-1.13.90/src/util/util.h:104:5: note: expanded from macro 'TEVENT_REQ_RETURN_ON_ERROR' # if (tevent_req_is_error(req, &TRROEstate, &TRROEerr)) { \ # ^ sssd-1.13.90/src/responder/secrets/proxy.c:550:5: note: Assuming 'TRROEstate' is equal to TEVENT_REQ_USER_ERROR # TEVENT_REQ_RETURN_ON_ERROR(req); # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ sssd-1.13.90/src/util/util.h:105:13: note: expanded from macro 'TEVENT_REQ_RETURN_ON_ERROR' # if (TRROEstate == TEVENT_REQ_USER_ERROR) { \ # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ sssd-1.13.90/src/responder/secrets/proxy.c:550:5: note: Taking true branch sssd-1.13.90/src/util/util.h:105:9: note: expanded from macro 'TEVENT_REQ_RETURN_ON_ERROR' # if (TRROEstate == TEVENT_REQ_USER_ERROR) { \ # ^ sssd-1.13.90/src/responder/secrets/proxy.c:931:11: note: Returning from 'proxy_http_req_recv' # ret = proxy_http_req_recv(subreq, state, &reply); # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ sssd-1.13.90/src/responder/secrets/proxy.c:934:9: note: Assuming 'ret' is equal to 0 # if (ret != EOK) { # ^~~~~~~~~~ sssd-1.13.90/src/responder/secrets/proxy.c:934:5: note: Taking false branch # if (ret != EOK) { # ^ sssd-1.13.90/src/responder/secrets/proxy.c:940:39: note: Dereference of undefined pointer value # reply->status_code, reply->reason_phrase, # ^~~~~~~~~~~~~~~~~~ # 938| # 939| ret = sec_http_reply_with_headers(state->secreq, &state->secreq->reply, # 940|-> reply->status_code, reply->reason_phrase, # 941| reply->headers, reply->num_headers, # 942| &reply->body);
Thanks, IIRC the int-instead of enum use is intentional, I will look at the others.
Simo.
On Tue, 2016-04-05 at 12:57 -0400, Simo Sorce wrote:
Thanks, IIRC the int-instead of enum use is intentional, I will look at the others.
The last coverity/clang thing is a false positive, but I initialized reply to NULL anyway, I expect now it will start complaining of possible NULL dereference :-)
Attached find patches that fixes all other issues (hopefully), one of them simply dropped an entire function as it turned out I wasn't using it.
Simo.
On Tue, 2016-04-05 at 14:54 -0400, Simo Sorce wrote:
On Tue, 2016-04-05 at 12:57 -0400, Simo Sorce wrote:
Thanks, IIRC the int-instead of enum use is intentional, I will look at the others.
The last coverity/clang thing is a false positive, but I initialized reply to NULL anyway, I expect now it will start complaining of possible NULL dereference :-)
Attached find patches that fixes all other issues (hopefully), one of them simply dropped an entire function as it turned out I wasn't using it.
BUMP ?
Simo.
On Tue, Apr 05, 2016 at 02:54:10PM -0400, Simo Sorce wrote:
On Tue, 2016-04-05 at 12:57 -0400, Simo Sorce wrote:
Thanks, IIRC the int-instead of enum use is intentional, I will look at the others.
The last coverity/clang thing is a false positive, but I initialized reply to NULL anyway, I expect now it will start complaining of possible NULL dereference :-)
Attached find patches that fixes all other issues (hopefully), one of them simply dropped an entire function as it turned out I wasn't using it.
Simo.
-- Simo Sorce * Red Hat, Inc * New York
Ugh, it took me too long to get to this review properly. Sorry about this..
Since this patchset is large, I will send replies in batches.
From e215e5534bb56f3887521443ce6c77d13ea3518d Mon Sep 17 00:00:00 2001 From: Simo Sorce simo@redhat.com Date: Tue, 12 Jan 2016 20:07:59 -0500 Subject: [PATCH 01/15] Util: Add watchdog helper
ACK
I know Pavel Brezina also reviewed this patch earlier in a separate thread, so note to self: Add his RB :-)
From 0dff46755af6063ed4b0339020ae5bb686692de1 Mon Sep 17 00:00:00 2001 From: Simo Sorce simo@redhat.com Date: Tue, 12 Jan 2016 20:13:28 -0500 Subject: [PATCH 02/15] Server: Enable Watchdog in all daemons
This allows the services to self monitor.
Is it intentional that we also enable the watchdog in monitor? I haven't seen the sssd process being stuck and if it does, we probably have bigger issues, so it's probably fine, I just need to remember to not SIGSTOP sssd when testing anymore :)
Otherwise ack.
From c576e37c188ded932996c9714b18a71251ef1d63 Mon Sep 17 00:00:00 2001 From: Simo Sorce simo@redhat.com Date: Wed, 13 Jan 2016 11:51:09 -0500 Subject: [PATCH 03/15] Monitor: Remove ping infrastructure
Can we also remove the force_timeout option and the functions that use it? Looking at monitor.c, they are only called from a single failure situation which is ENOMEM, so I think it's actually a dead code, the probability we ever call it is very low.
If you agree with removing the force_timeout but you're busy, let me know and I can prepare a patch atop yours.
The only code that proved to be useful in the field of all this we are about to remove is the diag_cmd(). We could use it to run pstack on the 'stuck' child to learn where it was actually stuck. But I'm not sure it's possible to implement it since the watchdog handler is a "plain" signal handler and the diag_cmd() forks and execs...
That said, I still think it's worth removing the pings in favor of the watchdog, the pings proved to be too fragile..
Maybe it would be better to file a ticket to check again if systemd watchdog can be used in the future?
To be continued..
On Wed, Apr 20, 2016 at 09:59:57AM +0200, Jakub Hrozek wrote:
On Tue, Apr 05, 2016 at 02:54:10PM -0400, Simo Sorce wrote:
On Tue, 2016-04-05 at 12:57 -0400, Simo Sorce wrote:
Thanks, IIRC the int-instead of enum use is intentional, I will look at the others.
The last coverity/clang thing is a false positive, but I initialized reply to NULL anyway, I expect now it will start complaining of possible NULL dereference :-)
Attached find patches that fixes all other issues (hopefully), one of them simply dropped an entire function as it turned out I wasn't using it.
Simo.
-- Simo Sorce * Red Hat, Inc * New York
Ugh, it took me too long to get to this review properly. Sorry about this..
Since this patchset is large, I will send replies in batches.
From e215e5534bb56f3887521443ce6c77d13ea3518d Mon Sep 17 00:00:00 2001 From: Simo Sorce simo@redhat.com Date: Tue, 12 Jan 2016 20:07:59 -0500 Subject: [PATCH 01/15] Util: Add watchdog helper
ACK
I know Pavel Brezina also reviewed this patch earlier in a separate thread, so note to self: Add his RB :-)
From 0dff46755af6063ed4b0339020ae5bb686692de1 Mon Sep 17 00:00:00 2001 From: Simo Sorce simo@redhat.com Date: Tue, 12 Jan 2016 20:13:28 -0500 Subject: [PATCH 02/15] Server: Enable Watchdog in all daemons
This allows the services to self monitor.
Is it intentional that we also enable the watchdog in monitor? I haven't seen the sssd process being stuck and if it does, we probably have bigger issues, so it's probably fine, I just need to remember to not SIGSTOP sssd when testing anymore :)
Otherwise ack.
Actually, more questions...
Can you help me test this patch? I tried to inject sleep() into sssd_be code and the sleep was just interrupted by the SIGRT delivery. With SSSD, most of the time the process was stuck was because it was writing a lot of data with fsync()/fdatasync(). I can't find any information in the Linux fsync manpage on how fsync behaves wrt signals. openpub manpages indicate that fsync would return EINTR, which worries me a bit..
From c576e37c188ded932996c9714b18a71251ef1d63 Mon Sep 17 00:00:00 2001 From: Simo Sorce simo@redhat.com Date: Wed, 13 Jan 2016 11:51:09 -0500 Subject: [PATCH 03/15] Monitor: Remove ping infrastructure
Can we also remove the force_timeout option and the functions that use it? Looking at monitor.c, they are only called from a single failure situation which is ENOMEM, so I think it's actually a dead code, the probability we ever call it is very low.
If you agree with removing the force_timeout but you're busy, let me know and I can prepare a patch atop yours.
The only code that proved to be useful in the field of all this we are about to remove is the diag_cmd(). We could use it to run pstack on the 'stuck' child to learn where it was actually stuck. But I'm not sure it's possible to implement it since the watchdog handler is a "plain" signal handler and the diag_cmd() forks and execs...
After reading man 7 signal, it seems that fork() and exec() are OK to be called in a signal handler, but the question above is more urgent IMO..
Also, is there a reason to just deprecate the ping() method and not remove it right away?
That said, I still think it's worth removing the pings in favor of the watchdog, the pings proved to be too fragile..
Maybe it would be better to file a ticket to check again if systemd watchdog can be used in the future?
To be continued.. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
On Wed, Apr 20, 2016 at 10:32:59AM +0200, Jakub Hrozek wrote:
From 0dff46755af6063ed4b0339020ae5bb686692de1 Mon Sep 17 00:00:00 2001 From: Simo Sorce simo@redhat.com Date: Tue, 12 Jan 2016 20:13:28 -0500 Subject: [PATCH 02/15] Server: Enable Watchdog in all daemons
This allows the services to self monitor.
Is it intentional that we also enable the watchdog in monitor? I haven't seen the sssd process being stuck and if it does, we probably have bigger issues, so it's probably fine, I just need to remember to not SIGSTOP sssd when testing anymore :)
Otherwise ack.
Actually, more questions...
Can you help me test this patch? I tried to inject sleep() into sssd_be code and the sleep was just interrupted by the SIGRT delivery. With SSSD, most of the time the process was stuck was because it was writing a lot of data with fsync()/fdatasync(). I can't find any information in the Linux fsync manpage on how fsync behaves wrt signals. openpub manpages indicate that fsync would return EINTR, which worries me a bit..
Hmm, sorry, I was not being careful enough. man 7 signal also says: """ The sleep(3) function is also never restarted if interrupted by a handler, but gives a success return: the number of seconds remaining to sleep. """
so the sleep testcase was wrong even though CatchSignal uses SA_RESTART. But do you know how would write() or fsync() behave here? The signal manpage is a bit unclar to me as it talks about "slow" devices..
Or can you think of some easy way to test this?
On Wed, 2016-04-20 at 11:12 +0200, Jakub Hrozek wrote:
On Wed, Apr 20, 2016 at 10:32:59AM +0200, Jakub Hrozek wrote:
From 0dff46755af6063ed4b0339020ae5bb686692de1 Mon Sep 17 00:00:00 2001 From: Simo Sorce simo@redhat.com Date: Tue, 12 Jan 2016 20:13:28 -0500 Subject: [PATCH 02/15] Server: Enable Watchdog in all daemons
This allows the services to self monitor.
Is it intentional that we also enable the watchdog in monitor? I haven't seen the sssd process being stuck and if it does, we probably have bigger issues, so it's probably fine, I just need to remember to not SIGSTOP sssd when testing anymore :)
Otherwise ack.
Actually, more questions...
Can you help me test this patch? I tried to inject sleep() into sssd_be code and the sleep was just interrupted by the SIGRT delivery. With SSSD, most of the time the process was stuck was because it was writing a lot of data with fsync()/fdatasync(). I can't find any information in the Linux fsync manpage on how fsync behaves wrt signals. openpub manpages indicate that fsync would return EINTR, which worries me a bit..
Hmm, sorry, I was not being careful enough. man 7 signal also says: """ The sleep(3) function is also never restarted if interrupted by a handler, but gives a success return: the number of seconds remaining to sleep. """
so the sleep testcase was wrong even though CatchSignal uses SA_RESTART. But do you know how would write() or fsync() behave here? The signal manpage is a bit unclar to me as it talks about "slow" devices..
Or can you think of some easy way to test this?
The fsync manpage here says: "The call blocks until the device reports that the transfer has completed."
And does not report EINTR as a possible error.
That said I am a bit unclear what you want to test actually ?
Yes interruptible calls can be interrupted by a signal, that's always the case, if we have code that misbehave when a syscall is interrupted we need to fix that code.
Afaik when we write() we always check the return and retry on EINTR.
Simo.
On Wed, Apr 20, 2016 at 09:43:05AM -0400, Simo Sorce wrote:
On Wed, 2016-04-20 at 11:12 +0200, Jakub Hrozek wrote:
On Wed, Apr 20, 2016 at 10:32:59AM +0200, Jakub Hrozek wrote:
From 0dff46755af6063ed4b0339020ae5bb686692de1 Mon Sep 17 00:00:00 2001 From: Simo Sorce simo@redhat.com Date: Tue, 12 Jan 2016 20:13:28 -0500 Subject: [PATCH 02/15] Server: Enable Watchdog in all daemons
This allows the services to self monitor.
Is it intentional that we also enable the watchdog in monitor? I haven't seen the sssd process being stuck and if it does, we probably have bigger issues, so it's probably fine, I just need to remember to not SIGSTOP sssd when testing anymore :)
Otherwise ack.
Actually, more questions...
Can you help me test this patch? I tried to inject sleep() into sssd_be code and the sleep was just interrupted by the SIGRT delivery. With SSSD, most of the time the process was stuck was because it was writing a lot of data with fsync()/fdatasync(). I can't find any information in the Linux fsync manpage on how fsync behaves wrt signals. openpub manpages indicate that fsync would return EINTR, which worries me a bit..
Hmm, sorry, I was not being careful enough. man 7 signal also says: """ The sleep(3) function is also never restarted if interrupted by a handler, but gives a success return: the number of seconds remaining to sleep. """
so the sleep testcase was wrong even though CatchSignal uses SA_RESTART. But do you know how would write() or fsync() behave here? The signal manpage is a bit unclar to me as it talks about "slow" devices..
Or can you think of some easy way to test this?
The fsync manpage here says: "The call blocks until the device reports that the transfer has completed."
And does not report EINTR as a possible error.
That said I am a bit unclear what you want to test actually ?
I want to actually test that the service can be restarted if stuck and reconnects fine. So far I haven't been lucky - SIGSTOP-ing the service stopped delivery of the signals, so did attaching gdb and waiting.
But most importantly I want to make sure that if tdb is writing a transaction and the signal is delivered, then we don't fsync() in tdb doesn't get interrupted and doesn't corrupt the database. From all the cases where users complained about a service being restarted, it was always about tdb writing stuff to disk...
Yes interruptible calls can be interrupted by a signal, that's always the case, if we have code that misbehave when a syscall is interrupted we need to fix that code.
Afaik when we write() we always check the return and retry on EINTR.
Simo.
-- Simo Sorce * Red Hat, Inc * New York
On Wed, 2016-04-20 at 17:18 +0200, Jakub Hrozek wrote:
On Wed, Apr 20, 2016 at 09:43:05AM -0400, Simo Sorce wrote:
On Wed, 2016-04-20 at 11:12 +0200, Jakub Hrozek wrote:
On Wed, Apr 20, 2016 at 10:32:59AM +0200, Jakub Hrozek wrote:
From 0dff46755af6063ed4b0339020ae5bb686692de1 Mon Sep 17 00:00:00 2001 From: Simo Sorce simo@redhat.com Date: Tue, 12 Jan 2016 20:13:28 -0500 Subject: [PATCH 02/15] Server: Enable Watchdog in all daemons
This allows the services to self monitor.
Is it intentional that we also enable the watchdog in monitor? I haven't seen the sssd process being stuck and if it does, we probably have bigger issues, so it's probably fine, I just need to remember to not SIGSTOP sssd when testing anymore :)
Otherwise ack.
Actually, more questions...
Can you help me test this patch? I tried to inject sleep() into sssd_be code and the sleep was just interrupted by the SIGRT delivery. With SSSD, most of the time the process was stuck was because it was writing a lot of data with fsync()/fdatasync(). I can't find any information in the Linux fsync manpage on how fsync behaves wrt signals. openpub manpages indicate that fsync would return EINTR, which worries me a bit..
Hmm, sorry, I was not being careful enough. man 7 signal also says: """ The sleep(3) function is also never restarted if interrupted by a handler, but gives a success return: the number of seconds remaining to sleep. """
so the sleep testcase was wrong even though CatchSignal uses SA_RESTART. But do you know how would write() or fsync() behave here? The signal manpage is a bit unclar to me as it talks about "slow" devices..
Or can you think of some easy way to test this?
The fsync manpage here says: "The call blocks until the device reports that the transfer has completed."
And does not report EINTR as a possible error.
That said I am a bit unclear what you want to test actually ?
I want to actually test that the service can be restarted if stuck and reconnects fine. So far I haven't been lucky - SIGSTOP-ing the service stopped delivery of the signals, so did attaching gdb and waiting.
gdb it, do *not* cont, but tell gdb to not block signals, iirc you do that with: handle SIGRTMIN nostop noprint pass
if you then wait in gdb the timers will fire and eventually the process will be killed because you are blocking the main loop and not resetting the watchdog.
But most importantly I want to make sure that if tdb is writing a transaction and the signal is delivered, then we don't fsync() in tdb doesn't get interrupted and doesn't corrupt the database. From all the cases where users complained about a service being restarted, it was always about tdb writing stuff to disk...
I'm inquiring about this.
Simo.
On Wed, 2016-04-20 at 09:59 +0200, Jakub Hrozek wrote:
On Tue, Apr 05, 2016 at 02:54:10PM -0400, Simo Sorce wrote:
On Tue, 2016-04-05 at 12:57 -0400, Simo Sorce wrote:
Thanks, IIRC the int-instead of enum use is intentional, I will look at the others.
The last coverity/clang thing is a false positive, but I initialized reply to NULL anyway, I expect now it will start complaining of possible NULL dereference :-)
Attached find patches that fixes all other issues (hopefully), one of them simply dropped an entire function as it turned out I wasn't using it.
Simo.
-- Simo Sorce * Red Hat, Inc * New York
Ugh, it took me too long to get to this review properly. Sorry about this..
Since this patchset is large, I will send replies in batches.
From e215e5534bb56f3887521443ce6c77d13ea3518d Mon Sep 17 00:00:00 2001 From: Simo Sorce simo@redhat.com Date: Tue, 12 Jan 2016 20:07:59 -0500 Subject: [PATCH 01/15] Util: Add watchdog helper
ACK
I know Pavel Brezina also reviewed this patch earlier in a separate thread, so note to self: Add his RB :-)
From 0dff46755af6063ed4b0339020ae5bb686692de1 Mon Sep 17 00:00:00 2001 From: Simo Sorce simo@redhat.com Date: Tue, 12 Jan 2016 20:13:28 -0500 Subject: [PATCH 02/15] Server: Enable Watchdog in all daemons
This allows the services to self monitor.
Is it intentional that we also enable the watchdog in monitor?
I wasn?t intentional in the sense that i really wanted at all cost to have this behavior, but I see nothing wrong on sssd watchdogging itself.
I haven't seen the sssd process being stuck and if it does, we probably have bigger issues, so it's probably fine, I just need to remember to not SIGSTOP sssd when testing anymore :)
New debugging trick, as soon as you attach or execute a process run: p teardown_watchdog()
I have a gdb cmd file where I execute this as a matter of fact when attached to a sssd process.
Otherwise ack.
Thanks.
From c576e37c188ded932996c9714b18a71251ef1d63 Mon Sep 17 00:00:00 2001 From: Simo Sorce simo@redhat.com Date: Wed, 13 Jan 2016 11:51:09 -0500 Subject: [PATCH 03/15] Monitor: Remove ping infrastructure
Can we also remove the force_timeout option and the functions that use it? Looking at monitor.c, they are only called from a single failure situation which is ENOMEM, so I think it's actually a dead code, the probability we ever call it is very low.
If you agree with removing the force_timeout but you're busy, let me know and I can prepare a patch atop yours.
Please add patch on top.
The only code that proved to be useful in the field of all this we are about to remove is the diag_cmd(). We could use it to run pstack on the 'stuck' child to learn where it was actually stuck. But I'm not sure it's possible to implement it since the watchdog handler is a "plain" signal handler and the diag_cmd() forks and execs...
signal(7) sasys fork()/execve() are async-signal-safe functions, so it could be run from the handler directly, the other option is to kill(SIGXXX) the monitor, and then wait a few seconds before aborting, and hope the monitor has time to run pstack on the child for us.
Either way I'd consider writing a new patch for this later, and would consider internally dumping a stack trace to a debug file instead of executing an external process.
That said, I still think it's worth removing the pings in favor of the watchdog, the pings proved to be too fragile..
Not only fragile but also caused churn and wakeups in the system, as every watchdog beat forced at least a context switch and 2 processes to run.
Maybe it would be better to file a ticket to check again if systemd watchdog can be used in the future?
We can look at it, I didn't want to get too far down the systemd road with this patchset as the goal was another.
To be continued..
Ty, Simo.
On Tue, Apr 05, 2016 at 02:54:10PM -0400, Simo Sorce wrote:
On Tue, 2016-04-05 at 12:57 -0400, Simo Sorce wrote:
Thanks, IIRC the int-instead of enum use is intentional, I will look at the others.
The last coverity/clang thing is a false positive, but I initialized reply to NULL anyway, I expect now it will start complaining of possible NULL dereference :-)
Attached find patches that fixes all other issues (hopefully), one of them simply dropped an entire function as it turned out I wasn't using it.
Simo.
-- Simo Sorce * Red Hat, Inc * New York
From 02e259e44631d228e0ec8311392e4a1a1a661b89 Mon Sep 17 00:00:00 2001 From: Simo Sorce simo@redhat.com Date: Fri, 8 Jan 2016 17:51:06 -0500 Subject: [PATCH 04/15] Responders: Make the client context more generic
This patch breaks the PAM responder: Program received signal SIGSEGV, Segmentation fault. 0x0000000000000000 in ?? () (gdb) bt #0 0x0000000000000000 in ?? () #1 0x0000000000413512 in accept_fd_handler (ev=0x653a40, fde=0x668970, flags=1, ptr=0x6604b0) at /sssd/src/responder/common/responder_common.c:478 #2 0x00007f8fe25d5bf3 in epoll_event_loop (tvalp=0x7ffd4e5d41b0, epoll_ev=0x653c80) at ../tevent_epoll.c:728 #3 epoll_event_loop_once (ev=<optimized out>, location=<optimized out>) at ../tevent_epoll.c:926 #4 0x00007f8fe25d4137 in std_event_loop_once (ev=0x653a40, location=0x7f8fe5f88aed "/sssd/src/util/server.c:689") at ../tevent_standard.c:114 #5 0x00007f8fe25d038d in _tevent_loop_once (ev=ev@entry=0x653a40, location=location@entry=0x7f8fe5f88aed "/sssd/src/util/server.c:689") at ../tevent.c:533 #6 0x00007f8fe25d052b in tevent_common_loop_wait (ev=0x653a40, location=0x7f8fe5f88aed "/sssd/src/util/server.c:689") at ../tevent.c:637 #7 0x00007f8fe25d40d7 in std_event_loop_wait (ev=0x653a40, location=0x7f8fe5f88aed "/sssd/src/util/server.c:689") at ../tevent_standard.c:140 #8 0x00007f8fe5f67b55 in server_loop (main_ctx=0x654e90) at /sssd/src/util/server.c:689 #9 0x0000000000406f57 in main (argc=6, argv=0x7ffd4e5d4598) at /sssd/src/responder/pam/pamsrv.c:426 (gdb) frame 1 #1 0x0000000000413512 in accept_fd_handler (ev=0x653a40, fde=0x668970, flags=1, ptr=0x6604b0) at /sssd/src/responder/common/responder_common.c:478 478 ret = accept_ctx->connection_setup(cctx); (gdb) p accept_ctx $1 = (struct accept_fd_ctx *) 0x6604b0 (gdb) p accept_ctx->connection_setup $2 = (connection_setup_t) 0x0
Do you want me to fix this and proceed or will you?
The NSS, autofs and IFP responders seem to work fine. I haven't tested the others yet.
On Wed, 2016-04-20 at 11:55 +0200, Jakub Hrozek wrote:
On Tue, Apr 05, 2016 at 02:54:10PM -0400, Simo Sorce wrote:
On Tue, 2016-04-05 at 12:57 -0400, Simo Sorce wrote:
Thanks, IIRC the int-instead of enum use is intentional, I will look at the others.
The last coverity/clang thing is a false positive, but I initialized reply to NULL anyway, I expect now it will start complaining of possible NULL dereference :-)
Attached find patches that fixes all other issues (hopefully), one of them simply dropped an entire function as it turned out I wasn't using it.
Simo.
-- Simo Sorce * Red Hat, Inc * New York
From 02e259e44631d228e0ec8311392e4a1a1a661b89 Mon Sep 17 00:00:00 2001 From: Simo Sorce simo@redhat.com Date: Fri, 8 Jan 2016 17:51:06 -0500 Subject: [PATCH 04/15] Responders: Make the client context more generic
This patch breaks the PAM responder: Program received signal SIGSEGV, Segmentation fault. 0x0000000000000000 in ?? () (gdb) bt #0 0x0000000000000000 in ?? () #1 0x0000000000413512 in accept_fd_handler (ev=0x653a40, fde=0x668970, flags=1, ptr=0x6604b0) at /sssd/src/responder/common/responder_common.c:478 #2 0x00007f8fe25d5bf3 in epoll_event_loop (tvalp=0x7ffd4e5d41b0, epoll_ev=0x653c80) at ../tevent_epoll.c:728 #3 epoll_event_loop_once (ev=<optimized out>, location=<optimized out>) at ../tevent_epoll.c:926 #4 0x00007f8fe25d4137 in std_event_loop_once (ev=0x653a40, location=0x7f8fe5f88aed "/sssd/src/util/server.c:689") at ../tevent_standard.c:114 #5 0x00007f8fe25d038d in _tevent_loop_once (ev=ev@entry=0x653a40, location=location@entry=0x7f8fe5f88aed "/sssd/src/util/server.c:689") at ../tevent.c:533 #6 0x00007f8fe25d052b in tevent_common_loop_wait (ev=0x653a40, location=0x7f8fe5f88aed "/sssd/src/util/server.c:689") at ../tevent.c:637 #7 0x00007f8fe25d40d7 in std_event_loop_wait (ev=0x653a40, location=0x7f8fe5f88aed "/sssd/src/util/server.c:689") at ../tevent_standard.c:140 #8 0x00007f8fe5f67b55 in server_loop (main_ctx=0x654e90) at /sssd/src/util/server.c:689 #9 0x0000000000406f57 in main (argc=6, argv=0x7ffd4e5d4598) at /sssd/src/responder/pam/pamsrv.c:426 (gdb) frame 1 #1 0x0000000000413512 in accept_fd_handler (ev=0x653a40, fde=0x668970, flags=1, ptr=0x6604b0) at /sssd/src/responder/common/responder_common.c:478 478 ret = accept_ctx->connection_setup(cctx); (gdb) p accept_ctx $1 = (struct accept_fd_ctx *) 0x6604b0 (gdb) p accept_ctx->connection_setup $2 = (connection_setup_t) 0x0
Do you want me to fix this and proceed or will you?
If you already know what's wrong, feel free to fix it, if you have to spend time analyzing I can do it, should be an easy fix.
The NSS, autofs and IFP responders seem to work fine. I haven't tested the others yet.
Ok, sorry for that, I did install SSSD at various times, but was more concentrated on testing the secrets stuff, and the tests went smoothly, but I now realize they fake the connection handling.
Simo.
On Wed, Apr 20, 2016 at 09:57:03AM -0400, Simo Sorce wrote:
On Wed, 2016-04-20 at 11:55 +0200, Jakub Hrozek wrote:
On Tue, Apr 05, 2016 at 02:54:10PM -0400, Simo Sorce wrote:
On Tue, 2016-04-05 at 12:57 -0400, Simo Sorce wrote:
Thanks, IIRC the int-instead of enum use is intentional, I will look at the others.
The last coverity/clang thing is a false positive, but I initialized reply to NULL anyway, I expect now it will start complaining of possible NULL dereference :-)
Attached find patches that fixes all other issues (hopefully), one of them simply dropped an entire function as it turned out I wasn't using it.
Simo.
-- Simo Sorce * Red Hat, Inc * New York
From 02e259e44631d228e0ec8311392e4a1a1a661b89 Mon Sep 17 00:00:00 2001 From: Simo Sorce simo@redhat.com Date: Fri, 8 Jan 2016 17:51:06 -0500 Subject: [PATCH 04/15] Responders: Make the client context more generic
This patch breaks the PAM responder: Program received signal SIGSEGV, Segmentation fault. 0x0000000000000000 in ?? () (gdb) bt #0 0x0000000000000000 in ?? () #1 0x0000000000413512 in accept_fd_handler (ev=0x653a40, fde=0x668970, flags=1, ptr=0x6604b0) at /sssd/src/responder/common/responder_common.c:478 #2 0x00007f8fe25d5bf3 in epoll_event_loop (tvalp=0x7ffd4e5d41b0, epoll_ev=0x653c80) at ../tevent_epoll.c:728 #3 epoll_event_loop_once (ev=<optimized out>, location=<optimized out>) at ../tevent_epoll.c:926 #4 0x00007f8fe25d4137 in std_event_loop_once (ev=0x653a40, location=0x7f8fe5f88aed "/sssd/src/util/server.c:689") at ../tevent_standard.c:114 #5 0x00007f8fe25d038d in _tevent_loop_once (ev=ev@entry=0x653a40, location=location@entry=0x7f8fe5f88aed "/sssd/src/util/server.c:689") at ../tevent.c:533 #6 0x00007f8fe25d052b in tevent_common_loop_wait (ev=0x653a40, location=0x7f8fe5f88aed "/sssd/src/util/server.c:689") at ../tevent.c:637 #7 0x00007f8fe25d40d7 in std_event_loop_wait (ev=0x653a40, location=0x7f8fe5f88aed "/sssd/src/util/server.c:689") at ../tevent_standard.c:140 #8 0x00007f8fe5f67b55 in server_loop (main_ctx=0x654e90) at /sssd/src/util/server.c:689 #9 0x0000000000406f57 in main (argc=6, argv=0x7ffd4e5d4598) at /sssd/src/responder/pam/pamsrv.c:426 (gdb) frame 1 #1 0x0000000000413512 in accept_fd_handler (ev=0x653a40, fde=0x668970, flags=1, ptr=0x6604b0) at /sssd/src/responder/common/responder_common.c:478 478 ret = accept_ctx->connection_setup(cctx); (gdb) p accept_ctx $1 = (struct accept_fd_ctx *) 0x6604b0 (gdb) p accept_ctx->connection_setup $2 = (connection_setup_t) 0x0
Do you want me to fix this and proceed or will you?
If you already know what's wrong, feel free to fix it, if you have to spend time analyzing I can do it, should be an easy fix.
Yes, I can prepare a branch on github with fixup patches and ask you if it's OK to squash the fixups into your patches.
The NSS, autofs and IFP responders seem to work fine. I haven't tested the others yet.
Ok, sorry for that, I did install SSSD at various times, but was more concentrated on testing the secrets stuff, and the tests went smoothly, but I now realize they fake the connection handling.
Simo.
-- Simo Sorce * Red Hat, Inc * New York
On Tue, Apr 05, 2016 at 02:54:10PM -0400, Simo Sorce wrote:
On Tue, 2016-04-05 at 12:57 -0400, Simo Sorce wrote:
Thanks, IIRC the int-instead of enum use is intentional, I will look at the others.
The last coverity/clang thing is a false positive, but I initialized reply to NULL anyway, I expect now it will start complaining of possible NULL dereference :-)
Attached find patches that fixes all other issues (hopefully), one of them simply dropped an entire function as it turned out I wasn't using it.
Simo.
-- Simo Sorce * Red Hat, Inc * New York
From 4610b546cb37a150ebaee12559c19a17e422708c Mon Sep 17 00:00:00 2001 From: Simo Sorce simo@redhat.com Date: Thu, 7 Jan 2016 10:17:38 -0500 Subject: [PATCH 05/15] Responders: Add support for socket activation
ACK (visual at this point) with a question - do we want to check that the fd we received is a UNIX socket using sd_is_socket_unix()?
The sd_listen_fds() manpage recommends that.
From 3755b157de1309f554a380e58c42c38dcd9cc5aa Mon Sep 17 00:00:00 2001 From: Simo Sorce simo@redhat.com Date: Wed, 20 Jan 2016 10:33:39 -0500 Subject: [PATCH 06/15] ConfDB: Add helper function to get "subsections"
The secrets database will have "subsections", ie sections that are in the "secrets" namespace and look like this: [secrets/<path>]
This function allows to source any section under secrets/ or under any arbitrary sub-path.
Related: https://fedorahosted.org/sssd/ticket/2913
src/confdb/confdb.c | 92 +++++++++++++++++++++++++++++++++++++++++++++++++++++ src/confdb/confdb.h | 26 +++++++++++++++ 2 files changed, 118 insertions(+)
diff --git a/src/confdb/confdb.c b/src/confdb/confdb.c index d409344890c869aa3e7b2dbb49c0f51cd3a20adc..03adbe593b42f556b560df77d410f80460200a67 100644 --- a/src/confdb/confdb.c +++ b/src/confdb/confdb.c @@ -1531,3 +1531,95 @@ done: talloc_free(tmp_ctx); return ret; }
+int confdb_get_sub_sections(TALLOC_CTX *mem_ctx,
struct confdb_ctx *cdb,const char *section,char ***sections,int *num_sections)+{
- TALLOC_CTX *tmp_ctx = NULL;
- char *secdn;
- struct ldb_dn *base = NULL;
- struct ldb_result *res = NULL;
- static const char *attrs[] = {"cn", NULL};
- char **names;
- int base_comp_num;
- int num;
- int i;
Can you use size_t here so that clang doesn't complain about "comparison of integers of different signs: 'int' and 'unsigned int'" in the for loop below?
- int ret;
- tmp_ctx = talloc_new(mem_ctx);
- if (tmp_ctx == NULL) {
return ENOMEM;- }
- ret = parse_section(tmp_ctx, section, &secdn, NULL);
- if (ret != EOK) {
goto done;- }
- base = ldb_dn_new(tmp_ctx, cdb->ldb, secdn);
- if (base == NULL) {
ret = ENOMEM;goto done;- }
- base_comp_num = ldb_dn_get_comp_num(base);
- ret = ldb_search(cdb->ldb, tmp_ctx, &res, base, LDB_SCOPE_SUBTREE,
attrs, NULL);- if (ret != LDB_SUCCESS) {
ret = EIO;goto done;- }
- names = talloc_zero_array(tmp_ctx, char *, res->count + 1);
- if (names == NULL) {
ret = ENOMEM;goto done;- }
- for (num = 0, i = 0; i < res->count; i++) {
const struct ldb_val *val;char *name;int n;int j;
Every time I see variables declared in a scope in C except loop control variables I think "This should be a static function of its own" :-)
n = ldb_dn_get_comp_num(res->msgs[i]->dn);if (n == base_comp_num) continue;name = NULL;for (j = n - base_comp_num - 1; j >= 0; j--) {val = ldb_dn_get_component_val(res->msgs[i]->dn, j);if (name == NULL) {name = talloc_strndup(names,(const char *)val->data, val->length);} else {name = talloc_asprintf(names, "%s/%.*s", name,(int)val->length,(const char *)val->data);}if (name == NULL) {ret = ENOMEM;goto done;}}names[num] = name;if (names[num] == NULL) {ret = ENOMEM;goto done;}num++;- }
From 082d09cac5919d651accda2d4163deb022fcb7f6 Mon Sep 17 00:00:00 2001 From: Simo Sorce simo@redhat.com Date: Fri, 8 Jan 2016 10:56:17 -0500 Subject: [PATCH 07/15] Secrets: Add autoconf macros to build with secrets
Prepares autoconf for the new Secrets Provider
ACK
From aa6203a0a6cb1f3ac60428887e77fe176489c3e0 Mon Sep 17 00:00:00 2001 From: Christian Heimes cheimes@redhat.com Date: Fri, 8 Jan 2016 13:26:22 +0100 Subject: [PATCH 08/15] Secrets: m4 macros for jansson and http-parser
Prepares autoconf for the new Secrets Provider dependencies
[...]
+PKG_CHECK_MODULES([HTTP_PARSER], [http_parser], [found_http_parser=yes], [found_http_parser=no])
There is no pkgconfig for http-parser-devel, so it seems to be this line is redundant.
Otherwise ACK.
On 04/20/2016 02:16 PM, Jakub Hrozek wrote:
- for (num = 0, i = 0; i < res->count; i++) {
const struct ldb_val *val;char *name;int n;int j;Every time I see variables declared in a scope in C except loop control variables I think "This should be a static function of its own":-)
Yes, and I think it's great that Simo does so, instead of hiding this fact by placing all variables at the beginning of the function. Limiting scope of variables as much as possible is a good think!
On Wed, 2016-04-20 at 14:16 +0200, Jakub Hrozek wrote:
On Tue, Apr 05, 2016 at 02:54:10PM -0400, Simo Sorce wrote:
On Tue, 2016-04-05 at 12:57 -0400, Simo Sorce wrote:
Thanks, IIRC the int-instead of enum use is intentional, I will look at the others.
The last coverity/clang thing is a false positive, but I initialized reply to NULL anyway, I expect now it will start complaining of possible NULL dereference :-)
Attached find patches that fixes all other issues (hopefully), one of them simply dropped an entire function as it turned out I wasn't using it.
Simo.
-- Simo Sorce * Red Hat, Inc * New York
From 4610b546cb37a150ebaee12559c19a17e422708c Mon Sep 17 00:00:00 2001 From: Simo Sorce simo@redhat.com Date: Thu, 7 Jan 2016 10:17:38 -0500 Subject: [PATCH 05/15] Responders: Add support for socket activation
ACK (visual at this point) with a question - do we want to check that the fd we received is a UNIX socket using sd_is_socket_unix()?
The sd_listen_fds() manpage recommends that.
If they recommend it we should, yes.
From 3755b157de1309f554a380e58c42c38dcd9cc5aa Mon Sep 17 00:00:00 2001 From: Simo Sorce simo@redhat.com Date: Wed, 20 Jan 2016 10:33:39 -0500 Subject: [PATCH 06/15] ConfDB: Add helper function to get "subsections"
The secrets database will have "subsections", ie sections that are in the "secrets" namespace and look like this: [secrets/<path>]
This function allows to source any section under secrets/ or under any arbitrary sub-path.
Related: https://fedorahosted.org/sssd/ticket/2913
src/confdb/confdb.c | 92 +++++++++++++++++++++++++++++++++++++++++++++++++++++ src/confdb/confdb.h | 26 +++++++++++++++ 2 files changed, 118 insertions(+)
diff --git a/src/confdb/confdb.c b/src/confdb/confdb.c index d409344890c869aa3e7b2dbb49c0f51cd3a20adc..03adbe593b42f556b560df77d410f80460200a67 100644 --- a/src/confdb/confdb.c +++ b/src/confdb/confdb.c @@ -1531,3 +1531,95 @@ done: talloc_free(tmp_ctx); return ret; }
+int confdb_get_sub_sections(TALLOC_CTX *mem_ctx,
struct confdb_ctx *cdb,const char *section,char ***sections,int *num_sections)+{
- TALLOC_CTX *tmp_ctx = NULL;
- char *secdn;
- struct ldb_dn *base = NULL;
- struct ldb_result *res = NULL;
- static const char *attrs[] = {"cn", NULL};
- char **names;
- int base_comp_num;
- int num;
- int i;
Can you use size_t here so that clang doesn't complain about "comparison of integers of different signs: 'int' and 'unsigned int'" in the for loop below?
meh, ok :-)
- int ret;
- tmp_ctx = talloc_new(mem_ctx);
- if (tmp_ctx == NULL) {
return ENOMEM;- }
- ret = parse_section(tmp_ctx, section, &secdn, NULL);
- if (ret != EOK) {
goto done;- }
- base = ldb_dn_new(tmp_ctx, cdb->ldb, secdn);
- if (base == NULL) {
ret = ENOMEM;goto done;- }
- base_comp_num = ldb_dn_get_comp_num(base);
- ret = ldb_search(cdb->ldb, tmp_ctx, &res, base, LDB_SCOPE_SUBTREE,
attrs, NULL);- if (ret != LDB_SUCCESS) {
ret = EIO;goto done;- }
- names = talloc_zero_array(tmp_ctx, char *, res->count + 1);
- if (names == NULL) {
ret = ENOMEM;goto done;- }
- for (num = 0, i = 0; i < res->count; i++) {
const struct ldb_val *val;char *name;int n;int j;Every time I see variables declared in a scope in C except loop control variables I think "This should be a static function of its own" :-)
Should it be in this case ? </lazy>
n = ldb_dn_get_comp_num(res->msgs[i]->dn);if (n == base_comp_num) continue;name = NULL;for (j = n - base_comp_num - 1; j >= 0; j--) {val = ldb_dn_get_component_val(res->msgs[i]->dn, j);if (name == NULL) {name = talloc_strndup(names,(const char *)val->data, val->length);} else {name = talloc_asprintf(names, "%s/%.*s", name,(int)val->length,(const char *)val->data);}if (name == NULL) {ret = ENOMEM;goto done;}}names[num] = name;if (names[num] == NULL) {ret = ENOMEM;goto done;}num++;- }
From 082d09cac5919d651accda2d4163deb022fcb7f6 Mon Sep 17 00:00:00 2001 From: Simo Sorce simo@redhat.com Date: Fri, 8 Jan 2016 10:56:17 -0500 Subject: [PATCH 07/15] Secrets: Add autoconf macros to build with secrets
Prepares autoconf for the new Secrets Provider
ACK
From aa6203a0a6cb1f3ac60428887e77fe176489c3e0 Mon Sep 17 00:00:00 2001 From: Christian Heimes cheimes@redhat.com Date: Fri, 8 Jan 2016 13:26:22 +0100 Subject: [PATCH 08/15] Secrets: m4 macros for jansson and http-parser
Prepares autoconf for the new Secrets Provider dependencies
[...]
+PKG_CHECK_MODULES([HTTP_PARSER], [http_parser], [found_http_parser=yes], [found_http_parser=no])
There is no pkgconfig for http-parser-devel, so it seems to be this line is redundant.
Otherwise ACK.
I wonder why this is not failing then ?
Simo.
On Wed, Apr 20, 2016 at 09:59:19AM -0400, Simo Sorce wrote:
On Wed, 2016-04-20 at 14:16 +0200, Jakub Hrozek wrote:
On Tue, Apr 05, 2016 at 02:54:10PM -0400, Simo Sorce wrote:
On Tue, 2016-04-05 at 12:57 -0400, Simo Sorce wrote:
Thanks, IIRC the int-instead of enum use is intentional, I will look at the others.
The last coverity/clang thing is a false positive, but I initialized reply to NULL anyway, I expect now it will start complaining of possible NULL dereference :-)
Attached find patches that fixes all other issues (hopefully), one of them simply dropped an entire function as it turned out I wasn't using it.
Simo.
-- Simo Sorce * Red Hat, Inc * New York
From 4610b546cb37a150ebaee12559c19a17e422708c Mon Sep 17 00:00:00 2001 From: Simo Sorce simo@redhat.com Date: Thu, 7 Jan 2016 10:17:38 -0500 Subject: [PATCH 05/15] Responders: Add support for socket activation
ACK (visual at this point) with a question - do we want to check that the fd we received is a UNIX socket using sd_is_socket_unix()?
The sd_listen_fds() manpage recommends that.
If they recommend it we should, yes.
OK, same as with the responder issue, I will prepare a fixup patch and ask you to check it before squashing into your patches..
From 3755b157de1309f554a380e58c42c38dcd9cc5aa Mon Sep 17 00:00:00 2001 From: Simo Sorce simo@redhat.com Date: Wed, 20 Jan 2016 10:33:39 -0500 Subject: [PATCH 06/15] ConfDB: Add helper function to get "subsections"
The secrets database will have "subsections", ie sections that are in the "secrets" namespace and look like this: [secrets/<path>]
This function allows to source any section under secrets/ or under any arbitrary sub-path.
[...]
+int confdb_get_sub_sections(TALLOC_CTX *mem_ctx,
struct confdb_ctx *cdb,const char *section,char ***sections,int *num_sections)+{
- TALLOC_CTX *tmp_ctx = NULL;
- char *secdn;
- struct ldb_dn *base = NULL;
- struct ldb_result *res = NULL;
- static const char *attrs[] = {"cn", NULL};
- char **names;
- int base_comp_num;
- int num;
- int i;
Can you use size_t here so that clang doesn't complain about "comparison of integers of different signs: 'int' and 'unsigned int'" in the for loop below?
meh, ok :-)
Trivial, I can also fix this locally and ask you if it's OK to squash.
- int ret;
- tmp_ctx = talloc_new(mem_ctx);
- if (tmp_ctx == NULL) {
return ENOMEM;- }
- ret = parse_section(tmp_ctx, section, &secdn, NULL);
- if (ret != EOK) {
goto done;- }
- base = ldb_dn_new(tmp_ctx, cdb->ldb, secdn);
- if (base == NULL) {
ret = ENOMEM;goto done;- }
- base_comp_num = ldb_dn_get_comp_num(base);
- ret = ldb_search(cdb->ldb, tmp_ctx, &res, base, LDB_SCOPE_SUBTREE,
attrs, NULL);- if (ret != LDB_SUCCESS) {
ret = EIO;goto done;- }
- names = talloc_zero_array(tmp_ctx, char *, res->count + 1);
- if (names == NULL) {
ret = ENOMEM;goto done;- }
- for (num = 0, i = 0; i < res->count; i++) {
const struct ldb_val *val;char *name;int n;int j;Every time I see variables declared in a scope in C except loop control variables I think "This should be a static function of its own" :-)
Should it be in this case ? </lazy>
Not sure, I'll make up my mind when I fix the other trivial issues.
[...]
From aa6203a0a6cb1f3ac60428887e77fe176489c3e0 Mon Sep 17 00:00:00 2001 From: Christian Heimes cheimes@redhat.com Date: Fri, 8 Jan 2016 13:26:22 +0100 Subject: [PATCH 08/15] Secrets: m4 macros for jansson and http-parser
Prepares autoconf for the new Secrets Provider dependencies
[...]
+PKG_CHECK_MODULES([HTTP_PARSER], [http_parser], [found_http_parser=yes], [found_http_parser=no])
There is no pkgconfig for http-parser-devel, so it seems to be this line is redundant.
Otherwise ACK.
I wonder why this is not failing then ?
Because we find the library using the next AC_CHECK_LIB call. Again, I will fixup this locally and send a branch for review later.
On (20/04/16 17:21), Jakub Hrozek wrote:
On Wed, Apr 20, 2016 at 09:59:19AM -0400, Simo Sorce wrote:
On Wed, 2016-04-20 at 14:16 +0200, Jakub Hrozek wrote:
On Tue, Apr 05, 2016 at 02:54:10PM -0400, Simo Sorce wrote:
On Tue, 2016-04-05 at 12:57 -0400, Simo Sorce wrote:
Thanks, IIRC the int-instead of enum use is intentional, I will look at the others.
The last coverity/clang thing is a false positive, but I initialized reply to NULL anyway, I expect now it will start complaining of possible NULL dereference :-)
Attached find patches that fixes all other issues (hopefully), one of them simply dropped an entire function as it turned out I wasn't using it.
Simo.
-- Simo Sorce * Red Hat, Inc * New York
From 4610b546cb37a150ebaee12559c19a17e422708c Mon Sep 17 00:00:00 2001 From: Simo Sorce simo@redhat.com Date: Thu, 7 Jan 2016 10:17:38 -0500 Subject: [PATCH 05/15] Responders: Add support for socket activation
ACK (visual at this point) with a question - do we want to check that the fd we received is a UNIX socket using sd_is_socket_unix()?
The sd_listen_fds() manpage recommends that.
If they recommend it we should, yes.
OK, same as with the responder issue, I will prepare a fixup patch and ask you to check it before squashing into your patches..
From 3755b157de1309f554a380e58c42c38dcd9cc5aa Mon Sep 17 00:00:00 2001 From: Simo Sorce simo@redhat.com Date: Wed, 20 Jan 2016 10:33:39 -0500 Subject: [PATCH 06/15] ConfDB: Add helper function to get "subsections"
The secrets database will have "subsections", ie sections that are in the "secrets" namespace and look like this: [secrets/<path>]
This function allows to source any section under secrets/ or under any arbitrary sub-path.
[...]
+int confdb_get_sub_sections(TALLOC_CTX *mem_ctx,
struct confdb_ctx *cdb,const char *section,char ***sections,int *num_sections)+{
- TALLOC_CTX *tmp_ctx = NULL;
- char *secdn;
- struct ldb_dn *base = NULL;
- struct ldb_result *res = NULL;
- static const char *attrs[] = {"cn", NULL};
- char **names;
- int base_comp_num;
- int num;
- int i;
Can you use size_t here so that clang doesn't complain about "comparison of integers of different signs: 'int' and 'unsigned int'" in the for loop below?
meh, ok :-)
Trivial, I can also fix this locally and ask you if it's OK to squash.
- int ret;
- tmp_ctx = talloc_new(mem_ctx);
- if (tmp_ctx == NULL) {
return ENOMEM;- }
- ret = parse_section(tmp_ctx, section, &secdn, NULL);
- if (ret != EOK) {
goto done;- }
- base = ldb_dn_new(tmp_ctx, cdb->ldb, secdn);
- if (base == NULL) {
ret = ENOMEM;goto done;- }
- base_comp_num = ldb_dn_get_comp_num(base);
- ret = ldb_search(cdb->ldb, tmp_ctx, &res, base, LDB_SCOPE_SUBTREE,
attrs, NULL);- if (ret != LDB_SUCCESS) {
ret = EIO;goto done;- }
- names = talloc_zero_array(tmp_ctx, char *, res->count + 1);
- if (names == NULL) {
ret = ENOMEM;goto done;- }
- for (num = 0, i = 0; i < res->count; i++) {
const struct ldb_val *val;char *name;int n;int j;Every time I see variables declared in a scope in C except loop control variables I think "This should be a static function of its own" :-)
Should it be in this case ? </lazy>
Not sure, I'll make up my mind when I fix the other trivial issues.
[...]
From aa6203a0a6cb1f3ac60428887e77fe176489c3e0 Mon Sep 17 00:00:00 2001 From: Christian Heimes cheimes@redhat.com Date: Fri, 8 Jan 2016 13:26:22 +0100 Subject: [PATCH 08/15] Secrets: m4 macros for jansson and http-parser
Prepares autoconf for the new Secrets Provider dependencies
[...]
+PKG_CHECK_MODULES([HTTP_PARSER], [http_parser], [found_http_parser=yes], [found_http_parser=no])
There is no pkgconfig for http-parser-devel, so it seems to be this line is redundant.
Otherwise ACK.
I wonder why this is not failing then ?
Because we find the library using the next AC_CHECK_LIB call. Again, I will fixup this locally and send a branch for review later.
We have PKG_CHECK_MODULES and then fallback to AC_CHECK_LIB AC_CHECK_HEADER on many places. It's not a problem from my POV.
If we would like to simplify it. We could get rid of fallback detection on many places. And http_parser can provide pkgconfig file in future.
I would prefer to be "kind of consistent" in detection. But I do not insist on it.
LS
On Wed, 2016-04-20 at 19:58 +0200, Lukas Slebodnik wrote:
On (20/04/16 17:21), Jakub Hrozek wrote:
On Wed, Apr 20, 2016 at 09:59:19AM -0400, Simo Sorce wrote:
On Wed, 2016-04-20 at 14:16 +0200, Jakub Hrozek wrote:
On Tue, Apr 05, 2016 at 02:54:10PM -0400, Simo Sorce wrote:
On Tue, 2016-04-05 at 12:57 -0400, Simo Sorce wrote:
Thanks, IIRC the int-instead of enum use is intentional, I will look at the others.
The last coverity/clang thing is a false positive, but I initialized reply to NULL anyway, I expect now it will start complaining of possible NULL dereference :-)
Attached find patches that fixes all other issues (hopefully), one of them simply dropped an entire function as it turned out I wasn't using it.
Simo.
-- Simo Sorce * Red Hat, Inc * New York
From 4610b546cb37a150ebaee12559c19a17e422708c Mon Sep 17 00:00:00 2001 From: Simo Sorce simo@redhat.com Date: Thu, 7 Jan 2016 10:17:38 -0500 Subject: [PATCH 05/15] Responders: Add support for socket activation
ACK (visual at this point) with a question - do we want to check that the fd we received is a UNIX socket using sd_is_socket_unix()?
The sd_listen_fds() manpage recommends that.
If they recommend it we should, yes.
OK, same as with the responder issue, I will prepare a fixup patch and ask you to check it before squashing into your patches..
From 3755b157de1309f554a380e58c42c38dcd9cc5aa Mon Sep 17 00:00:00 2001 From: Simo Sorce simo@redhat.com Date: Wed, 20 Jan 2016 10:33:39 -0500 Subject: [PATCH 06/15] ConfDB: Add helper function to get "subsections"
The secrets database will have "subsections", ie sections that are in the "secrets" namespace and look like this: [secrets/<path>]
This function allows to source any section under secrets/ or under any arbitrary sub-path.
[...]
+int confdb_get_sub_sections(TALLOC_CTX *mem_ctx,
struct confdb_ctx *cdb,const char *section,char ***sections,int *num_sections)+{
- TALLOC_CTX *tmp_ctx = NULL;
- char *secdn;
- struct ldb_dn *base = NULL;
- struct ldb_result *res = NULL;
- static const char *attrs[] = {"cn", NULL};
- char **names;
- int base_comp_num;
- int num;
- int i;
Can you use size_t here so that clang doesn't complain about "comparison of integers of different signs: 'int' and 'unsigned int'" in the for loop below?
meh, ok :-)
Trivial, I can also fix this locally and ask you if it's OK to squash.
- int ret;
- tmp_ctx = talloc_new(mem_ctx);
- if (tmp_ctx == NULL) {
return ENOMEM;- }
- ret = parse_section(tmp_ctx, section, &secdn, NULL);
- if (ret != EOK) {
goto done;- }
- base = ldb_dn_new(tmp_ctx, cdb->ldb, secdn);
- if (base == NULL) {
ret = ENOMEM;goto done;- }
- base_comp_num = ldb_dn_get_comp_num(base);
- ret = ldb_search(cdb->ldb, tmp_ctx, &res, base, LDB_SCOPE_SUBTREE,
attrs, NULL);- if (ret != LDB_SUCCESS) {
ret = EIO;goto done;- }
- names = talloc_zero_array(tmp_ctx, char *, res->count + 1);
- if (names == NULL) {
ret = ENOMEM;goto done;- }
- for (num = 0, i = 0; i < res->count; i++) {
const struct ldb_val *val;char *name;int n;int j;Every time I see variables declared in a scope in C except loop control variables I think "This should be a static function of its own" :-)
Should it be in this case ? </lazy>
Not sure, I'll make up my mind when I fix the other trivial issues.
[...]
From aa6203a0a6cb1f3ac60428887e77fe176489c3e0 Mon Sep 17 00:00:00 2001 From: Christian Heimes cheimes@redhat.com Date: Fri, 8 Jan 2016 13:26:22 +0100 Subject: [PATCH 08/15] Secrets: m4 macros for jansson and http-parser
Prepares autoconf for the new Secrets Provider dependencies
[...]
+PKG_CHECK_MODULES([HTTP_PARSER], [http_parser], [found_http_parser=yes], [found_http_parser=no])
There is no pkgconfig for http-parser-devel, so it seems to be this line is redundant.
Otherwise ACK.
I wonder why this is not failing then ?
Because we find the library using the next AC_CHECK_LIB call. Again, I will fixup this locally and send a branch for review later.
We have PKG_CHECK_MODULES and then fallback to AC_CHECK_LIB AC_CHECK_HEADER on many places. It's not a problem from my POV.
If we would like to simplify it. We could get rid of fallback detection on many places. And http_parser can provide pkgconfig file in future.
I would prefer to be "kind of consistent" in detection. But I do not insist on it.
I am not sure if you arguing to keep the checks as they are in my patch or to remove the pkgconfig based check :)
Simo.
On Wed, Apr 20, 2016 at 02:58:36PM -0400, Simo Sorce wrote:
On Wed, 2016-04-20 at 19:58 +0200, Lukas Slebodnik wrote:
On (20/04/16 17:21), Jakub Hrozek wrote:
On Wed, Apr 20, 2016 at 09:59:19AM -0400, Simo Sorce wrote:
On Wed, 2016-04-20 at 14:16 +0200, Jakub Hrozek wrote:
On Tue, Apr 05, 2016 at 02:54:10PM -0400, Simo Sorce wrote:
On Tue, 2016-04-05 at 12:57 -0400, Simo Sorce wrote: > Thanks, IIRC the int-instead of enum use is intentional, I will look > at the others.
The last coverity/clang thing is a false positive, but I initialized reply to NULL anyway, I expect now it will start complaining of possible NULL dereference :-)
Attached find patches that fixes all other issues (hopefully), one of them simply dropped an entire function as it turned out I wasn't using it.
Simo.
-- Simo Sorce * Red Hat, Inc * New York
From 4610b546cb37a150ebaee12559c19a17e422708c Mon Sep 17 00:00:00 2001 From: Simo Sorce simo@redhat.com Date: Thu, 7 Jan 2016 10:17:38 -0500 Subject: [PATCH 05/15] Responders: Add support for socket activation
ACK (visual at this point) with a question - do we want to check that the fd we received is a UNIX socket using sd_is_socket_unix()?
The sd_listen_fds() manpage recommends that.
If they recommend it we should, yes.
OK, same as with the responder issue, I will prepare a fixup patch and ask you to check it before squashing into your patches..
From 3755b157de1309f554a380e58c42c38dcd9cc5aa Mon Sep 17 00:00:00 2001 From: Simo Sorce simo@redhat.com Date: Wed, 20 Jan 2016 10:33:39 -0500 Subject: [PATCH 06/15] ConfDB: Add helper function to get "subsections"
The secrets database will have "subsections", ie sections that are in the "secrets" namespace and look like this: [secrets/<path>]
This function allows to source any section under secrets/ or under any arbitrary sub-path.
[...]
+int confdb_get_sub_sections(TALLOC_CTX *mem_ctx,
struct confdb_ctx *cdb,const char *section,char ***sections,int *num_sections)+{
- TALLOC_CTX *tmp_ctx = NULL;
- char *secdn;
- struct ldb_dn *base = NULL;
- struct ldb_result *res = NULL;
- static const char *attrs[] = {"cn", NULL};
- char **names;
- int base_comp_num;
- int num;
- int i;
Can you use size_t here so that clang doesn't complain about "comparison of integers of different signs: 'int' and 'unsigned int'" in the for loop below?
meh, ok :-)
Trivial, I can also fix this locally and ask you if it's OK to squash.
- int ret;
- tmp_ctx = talloc_new(mem_ctx);
- if (tmp_ctx == NULL) {
return ENOMEM;- }
- ret = parse_section(tmp_ctx, section, &secdn, NULL);
- if (ret != EOK) {
goto done;- }
- base = ldb_dn_new(tmp_ctx, cdb->ldb, secdn);
- if (base == NULL) {
ret = ENOMEM;goto done;- }
- base_comp_num = ldb_dn_get_comp_num(base);
- ret = ldb_search(cdb->ldb, tmp_ctx, &res, base, LDB_SCOPE_SUBTREE,
attrs, NULL);- if (ret != LDB_SUCCESS) {
ret = EIO;goto done;- }
- names = talloc_zero_array(tmp_ctx, char *, res->count + 1);
- if (names == NULL) {
ret = ENOMEM;goto done;- }
- for (num = 0, i = 0; i < res->count; i++) {
const struct ldb_val *val;char *name;int n;int j;Every time I see variables declared in a scope in C except loop control variables I think "This should be a static function of its own" :-)
Should it be in this case ? </lazy>
Not sure, I'll make up my mind when I fix the other trivial issues.
[...]
From aa6203a0a6cb1f3ac60428887e77fe176489c3e0 Mon Sep 17 00:00:00 2001 From: Christian Heimes cheimes@redhat.com Date: Fri, 8 Jan 2016 13:26:22 +0100 Subject: [PATCH 08/15] Secrets: m4 macros for jansson and http-parser
Prepares autoconf for the new Secrets Provider dependencies
[...]
+PKG_CHECK_MODULES([HTTP_PARSER], [http_parser], [found_http_parser=yes], [found_http_parser=no])
There is no pkgconfig for http-parser-devel, so it seems to be this line is redundant.
Otherwise ACK.
I wonder why this is not failing then ?
Because we find the library using the next AC_CHECK_LIB call. Again, I will fixup this locally and send a branch for review later.
We have PKG_CHECK_MODULES and then fallback to AC_CHECK_LIB AC_CHECK_HEADER on many places. It's not a problem from my POV.
If we would like to simplify it. We could get rid of fallback detection on many places. And http_parser can provide pkgconfig file in future.
I would prefer to be "kind of consistent" in detection. But I do not insist on it.
I am not sure if you arguing to keep the checks as they are in my patch or to remove the pkgconfig based check :)
Simo.
Given that Lukas says "http_parser can provide pkgconfig in future", I read his mail as a preference to keep the pkg-check test. And actually I agree, it doesn't hurt, let's keep it in.
On Thu, Apr 21, 2016 at 09:21:17AM +0200, Jakub Hrozek wrote:
Given that Lukas says "http_parser can provide pkgconfig in future", I read his mail as a preference to keep the pkg-check test. And actually I agree, it doesn't hurt, let's keep it in.
I wanted to push these patches: https://github.com/jhrozek/sssd/tree/secrets-review but..I can't find http_parser_strict on Debian, only http-parser and the patches seem to require the _strict version. I really don't know the difference between the two, can we fallback to the non-strict?
On Thu, 2016-06-23 at 21:36 +0200, Jakub Hrozek wrote:
On Thu, Apr 21, 2016 at 09:21:17AM +0200, Jakub Hrozek wrote:
Given that Lukas says "http_parser can provide pkgconfig in future", I read his mail as a preference to keep the pkg-check test. And actually I agree, it doesn't hurt, let's keep it in.
I wanted to push these patches: https://github.com/jhrozek/sssd/tree/secrets-review but..I can't find http_parser_strict on Debian, only http-parser and the patches seem to require the _strict version. I really don't know the difference between the two, can we fallback to the non-strict?
If it not too hard to detect if strict is present I would try to use it and fallback to not strict only of not available.
Strict *seems* a safer option.
Simo.
On Thu, Jun 23, 2016 at 04:23:12PM -0400, Simo Sorce wrote:
On Thu, 2016-06-23 at 21:36 +0200, Jakub Hrozek wrote:
On Thu, Apr 21, 2016 at 09:21:17AM +0200, Jakub Hrozek wrote:
Given that Lukas says "http_parser can provide pkgconfig in future", I read his mail as a preference to keep the pkg-check test. And actually I agree, it doesn't hurt, let's keep it in.
I wanted to push these patches: https://github.com/jhrozek/sssd/tree/secrets-review but..I can't find http_parser_strict on Debian, only http-parser and the patches seem to require the _strict version. I really don't know the difference between the two, can we fallback to the non-strict?
If it not too hard to detect if strict is present I would try to use it and fallback to not strict only of not available.
Thanks, I will prepare a fallback patch. I agree about the preference, but was wondering if we should require strict or not.
On (24/06/16 10:16), Jakub Hrozek wrote:
On Thu, Jun 23, 2016 at 04:23:12PM -0400, Simo Sorce wrote:
On Thu, 2016-06-23 at 21:36 +0200, Jakub Hrozek wrote:
On Thu, Apr 21, 2016 at 09:21:17AM +0200, Jakub Hrozek wrote:
Given that Lukas says "http_parser can provide pkgconfig in future", I read his mail as a preference to keep the pkg-check test. And actually I agree, it doesn't hurt, let's keep it in.
I wanted to push these patches: https://github.com/jhrozek/sssd/tree/secrets-review but..I can't find http_parser_strict on Debian, only http-parser and the patches seem to require the _strict version. I really don't know the difference between the two, can we fallback to the non-strict?
If it not too hard to detect if strict is present I would try to use it and fallback to not strict only of not available.
Thanks, I will prepare a fallback patch. I agree about the preference, but was wondering if we should require strict or not.
I did a very brief test of latest patches from github and I can see few errors/warnings.
src/responder/common/responder_common.c: In function ‘activate_unix_sockets’: src/responder/common/responder_common.c:795:13: error: too many arguments for format [-Werror=format-extra-args] DEBUG(SSSDBG_CRIT_FAILURE, ^~~~~ src/responder/common/responder_common.c:808:17: error: too many arguments for format [-Werror=format-extra-args] DEBUG(SSSDBG_CRIT_FAILURE, ^~~~~
src/responder/secrets/proxy.c: In function ‘ph_on_status’: src/responder/secrets/proxy.c:648:29: error: passing argument 2 of ‘ph_append_string’ from incompatible pointer type [-Werror=incompatible-pointer-types] ph_append_string(reply, &reply->reason_phrase, at, length); ^ src/responder/secrets/proxy.c:624:13: note: expected ‘char **’ but argument is of type ‘const char **’ static void ph_append_string(TALLOC_CTX *memctx, char **dest, ^~~~~~~~~~~~~~~~
CCLD test_cert_utils /usr/bin/ld: src/util/test_cert_utils-util.o: undefined reference to symbol 'hash_enter@@DHASH_0.4.3' /usr/lib64/libdhash.so.1: error adding symbols: DSO missing from command line Previous linking error is fixed by following diff diff --git a/Makefile.am b/Makefile.am index 622b0d2..7416141 100644 --- a/Makefile.am +++ b/Makefile.am @@ -2895,7 +2895,6 @@ test_krb5_wait_queue_LDADD = \
test_cert_utils_SOURCES = \ src/tests/cmocka/test_cert_utils.c \ - src/util/util.c \ $(NULL) test_cert_utils_CFLAGS = \ $(AM_CFLAGS) \
CCLD responder_common-tests /usr/bin/ld: ../../../src/responder/common/responder_common_tests-responder_common.o: undefined reference to symbol 'sd_listen_fds@@LIBSYSTEMD_209' /usr/lib64/libsystemd.so.0: error adding symbols: DSO missing from command line collect2: error: ld returned 1 exit status
CCLD negcache-tests /usr/bin/ld: ../../../src/responder/common/negcache_tests-responder_common.o: undefined reference to symbol 'sd_listen_fds@@LIBSYSTEMD_209' /usr/lib64/libsystemd.so.0: error adding symbols: DSO missing from command line collect2: error: ld returned 1 exit status
and previous two linking error with following diff diff --git a/src/tests/cwrap/Makefile.am b/src/tests/cwrap/Makefile.am index 8005d99..d8a49f1 100644 --- a/src/tests/cwrap/Makefile.am +++ b/src/tests/cwrap/Makefile.am @@ -145,6 +145,7 @@ responder_common_tests_LDADD = \ $(CMOCKA_LIBS) \ $(SSSD_LIBS) \ $(SELINUX_LIBS) \ + $(SYSTEMD_DAEMON_LIBS) \ $(abs_top_builddir)/libsss_util.la \ $(abs_top_builddir)/libsss_debug.la \ $(abs_top_builddir)/libsss_test_common.la \ @@ -162,6 +163,7 @@ negcache_tests_LDADD = \ $(CMOCKA_LIBS) \ $(SSSD_LIBS) \ $(SELINUX_LIBS) \ + $(SYSTEMD_DAEMON_LIBS) \ $(abs_top_builddir)/libsss_util.la \ $(abs_top_builddir)/libsss_debug.la \ $(abs_top_builddir)/libsss_test_common.la \
LS
On Wed, Jun 29, 2016 at 06:20:25PM +0200, Lukas Slebodnik wrote:
On (24/06/16 10:16), Jakub Hrozek wrote:
On Thu, Jun 23, 2016 at 04:23:12PM -0400, Simo Sorce wrote:
On Thu, 2016-06-23 at 21:36 +0200, Jakub Hrozek wrote:
On Thu, Apr 21, 2016 at 09:21:17AM +0200, Jakub Hrozek wrote:
Given that Lukas says "http_parser can provide pkgconfig in future", I read his mail as a preference to keep the pkg-check test. And actually I agree, it doesn't hurt, let's keep it in.
I wanted to push these patches: https://github.com/jhrozek/sssd/tree/secrets-review but..I can't find http_parser_strict on Debian, only http-parser and the patches seem to require the _strict version. I really don't know the difference between the two, can we fallback to the non-strict?
If it not too hard to detect if strict is present I would try to use it and fallback to not strict only of not available.
Thanks, I will prepare a fallback patch. I agree about the preference, but was wondering if we should require strict or not.
I did a very brief test of latest patches from github and I can see few errors/warnings.
src/responder/common/responder_common.c: In function ‘activate_unix_sockets’: src/responder/common/responder_common.c:795:13: error: too many arguments for format [-Werror=format-extra-args] DEBUG(SSSDBG_CRIT_FAILURE, ^~~~~ src/responder/common/responder_common.c:808:17: error: too many arguments for format [-Werror=format-extra-args] DEBUG(SSSDBG_CRIT_FAILURE, ^~~~~
src/responder/secrets/proxy.c: In function ‘ph_on_status’: src/responder/secrets/proxy.c:648:29: error: passing argument 2 of ‘ph_append_string’ from incompatible pointer type [-Werror=incompatible-pointer-types] ph_append_string(reply, &reply->reason_phrase, at, length); ^ src/responder/secrets/proxy.c:624:13: note: expected ‘char **’ but argument is of type ‘const char **’ static void ph_append_string(TALLOC_CTX *memctx, char **dest, ^~~~~~~~~~~~~~~~
CCLD test_cert_utils /usr/bin/ld: src/util/test_cert_utils-util.o: undefined reference to symbol 'hash_enter@@DHASH_0.4.3' /usr/lib64/libdhash.so.1: error adding symbols: DSO missing from command line Previous linking error is fixed by following diff diff --git a/Makefile.am b/Makefile.am index 622b0d2..7416141 100644 --- a/Makefile.am +++ b/Makefile.am @@ -2895,7 +2895,6 @@ test_krb5_wait_queue_LDADD = \
test_cert_utils_SOURCES = \ src/tests/cmocka/test_cert_utils.c \
- src/util/util.c \ $(NULL)
test_cert_utils_CFLAGS = \ $(AM_CFLAGS) \
CCLD responder_common-tests /usr/bin/ld: ../../../src/responder/common/responder_common_tests-responder_common.o: undefined reference to symbol 'sd_listen_fds@@LIBSYSTEMD_209' /usr/lib64/libsystemd.so.0: error adding symbols: DSO missing from command line collect2: error: ld returned 1 exit status
CCLD negcache-tests /usr/bin/ld: ../../../src/responder/common/negcache_tests-responder_common.o: undefined reference to symbol 'sd_listen_fds@@LIBSYSTEMD_209' /usr/lib64/libsystemd.so.0: error adding symbols: DSO missing from command line collect2: error: ld returned 1 exit status
and previous two linking error with following diff diff --git a/src/tests/cwrap/Makefile.am b/src/tests/cwrap/Makefile.am index 8005d99..d8a49f1 100644 --- a/src/tests/cwrap/Makefile.am +++ b/src/tests/cwrap/Makefile.am @@ -145,6 +145,7 @@ responder_common_tests_LDADD = \ $(CMOCKA_LIBS) \ $(SSSD_LIBS) \ $(SELINUX_LIBS) \
- $(SYSTEMD_DAEMON_LIBS) \ $(abs_top_builddir)/libsss_util.la \ $(abs_top_builddir)/libsss_debug.la \ $(abs_top_builddir)/libsss_test_common.la \
@@ -162,6 +163,7 @@ negcache_tests_LDADD = \ $(CMOCKA_LIBS) \ $(SSSD_LIBS) \ $(SELINUX_LIBS) \
- $(SYSTEMD_DAEMON_LIBS) \ $(abs_top_builddir)/libsss_util.la \ $(abs_top_builddir)/libsss_debug.la \ $(abs_top_builddir)/libsss_test_common.la \
OK, squashed and submitted to CI. Thanks, if the CI passes this time, I'll push the patches and *finally* release the beta.
On Wed, Jun 29, 2016 at 06:49:05PM +0200, Jakub Hrozek wrote:
On Wed, Jun 29, 2016 at 06:20:25PM +0200, Lukas Slebodnik wrote:
On (24/06/16 10:16), Jakub Hrozek wrote:
On Thu, Jun 23, 2016 at 04:23:12PM -0400, Simo Sorce wrote:
On Thu, 2016-06-23 at 21:36 +0200, Jakub Hrozek wrote:
On Thu, Apr 21, 2016 at 09:21:17AM +0200, Jakub Hrozek wrote:
Given that Lukas says "http_parser can provide pkgconfig in future", I read his mail as a preference to keep the pkg-check test. And actually I agree, it doesn't hurt, let's keep it in.
I wanted to push these patches: https://github.com/jhrozek/sssd/tree/secrets-review but..I can't find http_parser_strict on Debian, only http-parser and the patches seem to require the _strict version. I really don't know the difference between the two, can we fallback to the non-strict?
If it not too hard to detect if strict is present I would try to use it and fallback to not strict only of not available.
Thanks, I will prepare a fallback patch. I agree about the preference, but was wondering if we should require strict or not.
I did a very brief test of latest patches from github and I can see few errors/warnings.
src/responder/common/responder_common.c: In function ‘activate_unix_sockets’: src/responder/common/responder_common.c:795:13: error: too many arguments for format [-Werror=format-extra-args] DEBUG(SSSDBG_CRIT_FAILURE, ^~~~~ src/responder/common/responder_common.c:808:17: error: too many arguments for format [-Werror=format-extra-args] DEBUG(SSSDBG_CRIT_FAILURE, ^~~~~
src/responder/secrets/proxy.c: In function ‘ph_on_status’: src/responder/secrets/proxy.c:648:29: error: passing argument 2 of ‘ph_append_string’ from incompatible pointer type [-Werror=incompatible-pointer-types] ph_append_string(reply, &reply->reason_phrase, at, length); ^ src/responder/secrets/proxy.c:624:13: note: expected ‘char **’ but argument is of type ‘const char **’ static void ph_append_string(TALLOC_CTX *memctx, char **dest, ^~~~~~~~~~~~~~~~
CCLD test_cert_utils /usr/bin/ld: src/util/test_cert_utils-util.o: undefined reference to symbol 'hash_enter@@DHASH_0.4.3' /usr/lib64/libdhash.so.1: error adding symbols: DSO missing from command line Previous linking error is fixed by following diff diff --git a/Makefile.am b/Makefile.am index 622b0d2..7416141 100644 --- a/Makefile.am +++ b/Makefile.am @@ -2895,7 +2895,6 @@ test_krb5_wait_queue_LDADD = \
test_cert_utils_SOURCES = \ src/tests/cmocka/test_cert_utils.c \
- src/util/util.c \ $(NULL)
test_cert_utils_CFLAGS = \ $(AM_CFLAGS) \
CCLD responder_common-tests /usr/bin/ld: ../../../src/responder/common/responder_common_tests-responder_common.o: undefined reference to symbol 'sd_listen_fds@@LIBSYSTEMD_209' /usr/lib64/libsystemd.so.0: error adding symbols: DSO missing from command line collect2: error: ld returned 1 exit status
CCLD negcache-tests /usr/bin/ld: ../../../src/responder/common/negcache_tests-responder_common.o: undefined reference to symbol 'sd_listen_fds@@LIBSYSTEMD_209' /usr/lib64/libsystemd.so.0: error adding symbols: DSO missing from command line collect2: error: ld returned 1 exit status
and previous two linking error with following diff diff --git a/src/tests/cwrap/Makefile.am b/src/tests/cwrap/Makefile.am index 8005d99..d8a49f1 100644 --- a/src/tests/cwrap/Makefile.am +++ b/src/tests/cwrap/Makefile.am @@ -145,6 +145,7 @@ responder_common_tests_LDADD = \ $(CMOCKA_LIBS) \ $(SSSD_LIBS) \ $(SELINUX_LIBS) \
- $(SYSTEMD_DAEMON_LIBS) \ $(abs_top_builddir)/libsss_util.la \ $(abs_top_builddir)/libsss_debug.la \ $(abs_top_builddir)/libsss_test_common.la \
@@ -162,6 +163,7 @@ negcache_tests_LDADD = \ $(CMOCKA_LIBS) \ $(SSSD_LIBS) \ $(SELINUX_LIBS) \
- $(SYSTEMD_DAEMON_LIBS) \ $(abs_top_builddir)/libsss_util.la \ $(abs_top_builddir)/libsss_debug.la \ $(abs_top_builddir)/libsss_test_common.la \
OK, squashed and submitted to CI. Thanks, if the CI passes this time, I'll push the patches and *finally* release the beta.
Our public CI log viewer machine has some issues, so I sent the CI link which points directly to our Jenkins instance to our internal list.
ACK
For posterity, attached are patches I'm about to push.
On Wed, Jun 29, 2016 at 10:39:17PM +0200, Jakub Hrozek wrote:
On Wed, Jun 29, 2016 at 06:49:05PM +0200, Jakub Hrozek wrote:
On Wed, Jun 29, 2016 at 06:20:25PM +0200, Lukas Slebodnik wrote:
On (24/06/16 10:16), Jakub Hrozek wrote:
On Thu, Jun 23, 2016 at 04:23:12PM -0400, Simo Sorce wrote:
On Thu, 2016-06-23 at 21:36 +0200, Jakub Hrozek wrote:
On Thu, Apr 21, 2016 at 09:21:17AM +0200, Jakub Hrozek wrote: > Given that Lukas says "http_parser can provide pkgconfig in future", I > read his mail as a preference to keep the pkg-check test. And actually I > agree, it doesn't hurt, let's keep it in.
I wanted to push these patches: https://github.com/jhrozek/sssd/tree/secrets-review but..I can't find http_parser_strict on Debian, only http-parser and the patches seem to require the _strict version. I really don't know the difference between the two, can we fallback to the non-strict?
If it not too hard to detect if strict is present I would try to use it and fallback to not strict only of not available.
Thanks, I will prepare a fallback patch. I agree about the preference, but was wondering if we should require strict or not.
I did a very brief test of latest patches from github and I can see few errors/warnings.
src/responder/common/responder_common.c: In function ‘activate_unix_sockets’: src/responder/common/responder_common.c:795:13: error: too many arguments for format [-Werror=format-extra-args] DEBUG(SSSDBG_CRIT_FAILURE, ^~~~~ src/responder/common/responder_common.c:808:17: error: too many arguments for format [-Werror=format-extra-args] DEBUG(SSSDBG_CRIT_FAILURE, ^~~~~
src/responder/secrets/proxy.c: In function ‘ph_on_status’: src/responder/secrets/proxy.c:648:29: error: passing argument 2 of ‘ph_append_string’ from incompatible pointer type [-Werror=incompatible-pointer-types] ph_append_string(reply, &reply->reason_phrase, at, length); ^ src/responder/secrets/proxy.c:624:13: note: expected ‘char **’ but argument is of type ‘const char **’ static void ph_append_string(TALLOC_CTX *memctx, char **dest, ^~~~~~~~~~~~~~~~
CCLD test_cert_utils /usr/bin/ld: src/util/test_cert_utils-util.o: undefined reference to symbol 'hash_enter@@DHASH_0.4.3' /usr/lib64/libdhash.so.1: error adding symbols: DSO missing from command line Previous linking error is fixed by following diff diff --git a/Makefile.am b/Makefile.am index 622b0d2..7416141 100644 --- a/Makefile.am +++ b/Makefile.am @@ -2895,7 +2895,6 @@ test_krb5_wait_queue_LDADD = \
test_cert_utils_SOURCES = \ src/tests/cmocka/test_cert_utils.c \
- src/util/util.c \ $(NULL)
test_cert_utils_CFLAGS = \ $(AM_CFLAGS) \
CCLD responder_common-tests /usr/bin/ld: ../../../src/responder/common/responder_common_tests-responder_common.o: undefined reference to symbol 'sd_listen_fds@@LIBSYSTEMD_209' /usr/lib64/libsystemd.so.0: error adding symbols: DSO missing from command line collect2: error: ld returned 1 exit status
CCLD negcache-tests /usr/bin/ld: ../../../src/responder/common/negcache_tests-responder_common.o: undefined reference to symbol 'sd_listen_fds@@LIBSYSTEMD_209' /usr/lib64/libsystemd.so.0: error adding symbols: DSO missing from command line collect2: error: ld returned 1 exit status
and previous two linking error with following diff diff --git a/src/tests/cwrap/Makefile.am b/src/tests/cwrap/Makefile.am index 8005d99..d8a49f1 100644 --- a/src/tests/cwrap/Makefile.am +++ b/src/tests/cwrap/Makefile.am @@ -145,6 +145,7 @@ responder_common_tests_LDADD = \ $(CMOCKA_LIBS) \ $(SSSD_LIBS) \ $(SELINUX_LIBS) \
- $(SYSTEMD_DAEMON_LIBS) \ $(abs_top_builddir)/libsss_util.la \ $(abs_top_builddir)/libsss_debug.la \ $(abs_top_builddir)/libsss_test_common.la \
@@ -162,6 +163,7 @@ negcache_tests_LDADD = \ $(CMOCKA_LIBS) \ $(SSSD_LIBS) \ $(SELINUX_LIBS) \
- $(SYSTEMD_DAEMON_LIBS) \ $(abs_top_builddir)/libsss_util.la \ $(abs_top_builddir)/libsss_debug.la \ $(abs_top_builddir)/libsss_test_common.la \
OK, squashed and submitted to CI. Thanks, if the CI passes this time, I'll push the patches and *finally* release the beta.
Our public CI log viewer machine has some issues, so I sent the CI link which points directly to our Jenkins instance to our internal list.
ACK
For posterity, attached are patches I'm about to push.
Pushed the whole set as: * 2aafa4811cd9f74f70820b0e266df8e4c7ed6ad7 * a9d46b86993ee8d87fddf0ba50665c0b1b78ebb7 * e625eb47a3091d92eda2271b123f8aab06227b63 * 8f2a34cc6964a1f80a1434e05315a7ae0bb5774e * 625bb2ddf15e8f305a53afa44e87f2146fa930af * 1dd679584241a0f9b29072c7eed1c5c5e4a577e4 * a8d1a344e580f29699aed9b88d87fc3c6f5d113b * 85bd23a89282c96487e64872ac955e271e445d0b * f0beb4e313970ffd075cd711ed6cfbac03ad5af6 * b1ce544568eff89f2263ae180e323f263f1cff3a * 052f8aa2034f7b091097dc5fdafc201b7d684525 * e5911e72198df96ec7cfe486ff66363c2297a5f7 * 4f3a9d837a55b49448eca3c713c85a406207e523 * 9a6d162cacfaf6946a1bf974b80b643d2a052d7a * ba5e2d5e261e5f3ac6ce00227595f7265d2c715e * f9f3dbc87d49bea05a54c2bcd9f7e40e4e8ab85a * 75ba524d356fed615a9c92152f64aebf0bdaf9c2 * 96a624877512ac352736047023b65b8688039ae1
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Lukas Slebodnik -
Pavel Reichl -
Simo Sorce