On (07/05/15 12:01), Jakub Hrozek wrote:
>Hi,
>
>the attached patch fixes
https://fedorahosted.org/sssd/ticket/2649.
>
>I couldn't reproduce the bug with regular testing, but only by overriding the
>return value from semanage_connect()
>From 4116f54398994ed15a5506b7927f24ffdca3d19c Mon Sep 17 00:00:00 2001
>From: Jakub Hrozek <jhrozek(a)redhat.com>
>Date: Wed, 6 May 2015 08:40:12 +0200
>Subject: [PATCH] SELINUX: Avoid disconnecting disconnected handle
>
>Resolves:
>
https://fedorahosted.org/sssd/ticket/2649
>
>libsemanage is very strict about its API usage and actually doesn't
>allow disconnecting a handle that is not connected. The unpatched code
>would fail with:
>
> selinux_child: handle.c:231: semanage_disconnect: Assertion `sh !=
> ((void *)0) && sh->funcs != ((void *)0) &&
sh->funcs->disconnect !=
> ((void *)0)' failed.
>
>If semanage_connect() failed.
>---
> src/util/sss_semanage.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
>diff --git a/src/util/sss_semanage.c b/src/util/sss_semanage.c
>index
01a2f41d8752e127f2aa1b72faa61c23f315edd7..d1d03988c05dc011dbd465051d50fe6acca4f845 100644
>--- a/src/util/sss_semanage.c
>+++ b/src/util/sss_semanage.c
>@@ -70,8 +70,13 @@ static void sss_semanage_error_callback(void *varg,
>
> static void sss_semanage_close(semanage_handle_t *handle)
> {
>- /* Calling disconnect on a disconnected handle is safe */
>- semanage_disconnect(handle);
>+ if (handle == NULL) {
>+ return; /* semanage uses asserts */
>+ }
>+
>+ if (semanage_is_connected(handle)) {
>+ semanage_disconnect(handle);
>+ }
> semanage_handle_destroy(handle);
Althought handle could be NULL for function semanage_handle_destroy.
The documentation is clear.
Deallocate all space associated with a semanage_handle_t, including
the pointer itself. CAUTION: this function does not disconnect
from the backend; be sure that a semanage_disconnect() was
previously called if the handle was connected. */
void semanage_handle_destroy(semanage_handle_t *);
But neither semanage_is_connected nor semanage_disconnect accept NULL.
There is an assert in code but documentation does not say so.
Disconnect from the manager given by the handle. If already
disconnected then this function does nothing. Return 0 if
disconnected properly or already disconnected, negative value on
error. */
int semanage_disconnect(semanage_handle_t *);
ACK
http://sssd-ci.duckdns.org/logs/job/14/97/summary.html
LS
Thanks, the comment in semanage header is exactly why we had the comment
in the code that I removed with this patch..
Pushed upstream:
* master: 589a8760b38d9e2dfa278764af12d59e1487fe07
* sssd-1-12: 2ed3ac5190cd8f92d671e00837a360bd92cf150c