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()
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@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
On Mon, May 11, 2015 at 09:34:01AM +0200, Lukas Slebodnik wrote:
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@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
sssd-devel@lists.fedorahosted.org