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 <s...@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-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
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.
--
Simo Sorce * Red Hat, Inc * New York
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
