[SSSD] [PATCH] GPO: Don't use stdout for output in gpo_child

Jakub Hrozek jhrozek at redhat.com
Wed Jan 7 10:56:24 UTC 2015


Hi,

the attached patches work around the Samba bug
https://bugzilla.samba.org/show_bug.cgi?id=11036

To reproduce, set a high "log level" in smb.conf and attempt to log in.
At least with RHEL-7.1's Samba version, the gpo_child output would
contain error messages unless there is a file called /root/.smb/smb.conf
exists. Then processing the buffer gpo_child sends to parent would fail.

I'm not thrilled about using such a heavy-weight change and
special-casing where gpo_child sends output but I couldn't think of a
different fix.

I'll include unit test for the child_common change along with the other
child_common tests (it's easier to build upon them).
-------------- next part --------------
>From cbb23c858b716fcafcdbb5bff340f605b345bc76 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Tue, 6 Jan 2015 16:54:44 +0100
Subject: [PATCH 1/3] GPO: Set libsmb debugging to stderr

libsmb logs to stdout by default. It's much more reasonable to log to
stderr by default.

Please also note:
    https://bugzilla.samba.org/show_bug.cgi?id=11036
and:
    https://fedorahosted.org/sssd/ticket/2544
---
 src/providers/ad/ad_gpo_child.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/providers/ad/ad_gpo_child.c b/src/providers/ad/ad_gpo_child.c
index ceb2faeb65e797c5b8493cd4bbe95690d848f28d..aa0ab0dc351c7a29c1526d810d5b3cbae5562c5a 100644
--- a/src/providers/ad/ad_gpo_child.c
+++ b/src/providers/ad/ad_gpo_child.c
@@ -601,6 +601,7 @@ perform_smb_operations(int cached_gpt_version,
         goto done;
     }
 
+    smbc_setOptionDebugToStderr(smbc_ctx, 1);
     smbc_setFunctionAuthData(smbc_ctx, sssd_krb_get_auth_data_fn);
     smbc_setOptionUseKerberos(smbc_ctx, 1);
 
-- 
1.9.3

-------------- next part --------------
>From ea994d5c6f1472660a072899806173e4bb01b1bf Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Wed, 7 Jan 2015 10:36:12 +0100
Subject: [PATCH 2/3] UTIL: Allow dup-ing child pipe to a different FD

Related to:
    https://fedorahosted.org/sssd/ticket/2544

Adds a new function exec_child_ex and moves setting the extra_argv[]
to exec_child_ex() along with specifying the input and output fds.
---
 src/providers/ad/ad_gpo.c               |  2 +-
 src/providers/ipa/ipa_selinux.c         |  3 +--
 src/providers/krb5/krb5_child_handler.c |  8 ++++----
 src/providers/ldap/sdap_child_helpers.c |  3 +--
 src/tests/cmocka/test_child_common.c    | 11 ++++++-----
 src/util/child_common.c                 | 22 ++++++++++++++++------
 src/util/child_common.h                 | 12 ++++++++++--
 7 files changed, 39 insertions(+), 22 deletions(-)

diff --git a/src/providers/ad/ad_gpo.c b/src/providers/ad/ad_gpo.c
index 6dbe0595f9079426a661e6e5816a79b77ac5023a..4f10a8818843dfa52faa982684d9d2737c72ba59 100644
--- a/src/providers/ad/ad_gpo.c
+++ b/src/providers/ad/ad_gpo.c
@@ -3963,7 +3963,7 @@ gpo_fork_child(struct tevent_req *req)
     if (pid == 0) { /* child */
         err = exec_child(state,
                          pipefd_to_child, pipefd_from_child,
-                         GPO_CHILD, gpo_child_debug_fd, NULL);
+                         GPO_CHILD, gpo_child_debug_fd);
         DEBUG(SSSDBG_CRIT_FAILURE, "Could not exec gpo_child: [%d][%s].\n",
               err, strerror(err));
         return err;
diff --git a/src/providers/ipa/ipa_selinux.c b/src/providers/ipa/ipa_selinux.c
index c4e70cfcb0748988d91fc1db57cf5a30d5365be4..133b679b6d518704ebb2bd901c64ac48170c9a0b 100644
--- a/src/providers/ipa/ipa_selinux.c
+++ b/src/providers/ipa/ipa_selinux.c
@@ -1049,8 +1049,7 @@ static errno_t selinux_fork_child(struct selinux_child_state *state)
     if (pid == 0) { /* child */
         ret = exec_child(state,
                          pipefd_to_child, pipefd_from_child,
-                         SELINUX_CHILD, selinux_child_debug_fd,
-                         NULL);
+                         SELINUX_CHILD, selinux_child_debug_fd);
         DEBUG(SSSDBG_CRIT_FAILURE, "Could not exec selinux_child: [%d][%s].\n",
               ret, sss_strerror(ret));
         return ret;
diff --git a/src/providers/krb5/krb5_child_handler.c b/src/providers/krb5/krb5_child_handler.c
index 1454d220fb294abc339df6e862154012a03fdca0..633cd917737d3f39526b049cc3d930b67f8b5c66 100644
--- a/src/providers/krb5/krb5_child_handler.c
+++ b/src/providers/krb5/krb5_child_handler.c
@@ -305,10 +305,10 @@ static errno_t fork_child(struct tevent_req *req)
     pid = fork();
 
     if (pid == 0) { /* child */
-        err = exec_child(state,
-                         pipefd_to_child, pipefd_from_child,
-                         KRB5_CHILD, state->kr->krb5_ctx->child_debug_fd,
-                         k5c_extra_args);
+        err = exec_child_ex(state,
+                            pipefd_to_child, pipefd_from_child,
+                            KRB5_CHILD, state->kr->krb5_ctx->child_debug_fd,
+                            k5c_extra_args, STDIN_FILENO, STDOUT_FILENO);
         if (err != EOK) {
             DEBUG(SSSDBG_CRIT_FAILURE, "Could not exec KRB5 child: [%d][%s].\n",
                       err, strerror(err));
diff --git a/src/providers/ldap/sdap_child_helpers.c b/src/providers/ldap/sdap_child_helpers.c
index b60891d2b41f9a359856eb22174128d7f07559fb..40010989021eb7cf77b96876b2d1c4119ed39163 100644
--- a/src/providers/ldap/sdap_child_helpers.c
+++ b/src/providers/ldap/sdap_child_helpers.c
@@ -108,8 +108,7 @@ static errno_t sdap_fork_child(struct tevent_context *ev,
     if (pid == 0) { /* child */
         err = exec_child(child,
                          pipefd_to_child, pipefd_from_child,
-                         LDAP_CHILD, ldap_child_debug_fd,
-                         NULL);
+                         LDAP_CHILD, ldap_child_debug_fd);
         DEBUG(SSSDBG_CRIT_FAILURE, "Could not exec LDAP child: [%d][%s].\n",
                                     err, strerror(err));
         return err;
diff --git a/src/tests/cmocka/test_child_common.c b/src/tests/cmocka/test_child_common.c
index ee6e297f58b0895b5401464bb3f9b5386a3e6411..23e14ce49ff75c8c14320be687b456cfcad1982e 100644
--- a/src/tests/cmocka/test_child_common.c
+++ b/src/tests/cmocka/test_child_common.c
@@ -89,7 +89,7 @@ void test_exec_child(void **state)
         ret = exec_child(child_tctx,
                          child_tctx->pipefd_to_child,
                          child_tctx->pipefd_from_child,
-                         CHILD_DIR"/"TEST_BIN, 2, NULL);
+                         CHILD_DIR"/"TEST_BIN, 2);
         assert_int_equal(ret, EOK);
     } else {
             do {
@@ -128,10 +128,11 @@ void test_exec_child_extra_args(void **state)
     child_pid = fork();
     assert_int_not_equal(child_pid, -1);
     if (child_pid == 0) {
-        ret = exec_child(child_tctx,
-                         child_tctx->pipefd_to_child,
-                         child_tctx->pipefd_from_child,
-                         CHILD_DIR"/"TEST_BIN, 2, extra_args);
+        ret = exec_child_ex(child_tctx,
+                            child_tctx->pipefd_to_child,
+                            child_tctx->pipefd_from_child,
+                            CHILD_DIR"/"TEST_BIN, 2, extra_args,
+                            STDIN_FILENO, STDOUT_FILENO);
         assert_int_equal(ret, EOK);
     } else {
             do {
diff --git a/src/util/child_common.c b/src/util/child_common.c
index d3f488d181e83245f8a3ec003b60671248745bd5..d632cd4ec24923da88b8eeabfc137ddeec3a6519 100644
--- a/src/util/child_common.c
+++ b/src/util/child_common.c
@@ -729,17 +729,18 @@ fail:
     return ret;
 }
 
-errno_t exec_child(TALLOC_CTX *mem_ctx,
-                   int *pipefd_to_child, int *pipefd_from_child,
-                   const char *binary, int debug_fd,
-                   const char *extra_argv[])
+errno_t exec_child_ex(TALLOC_CTX *mem_ctx,
+                      int *pipefd_to_child, int *pipefd_from_child,
+                      const char *binary, int debug_fd,
+                      const char *extra_argv[],
+                      int child_in_fd, int child_out_fd)
 {
     int ret;
     errno_t err;
     char **argv;
 
     close(pipefd_to_child[1]);
-    ret = dup2(pipefd_to_child[0], STDIN_FILENO);
+    ret = dup2(pipefd_to_child[0], child_in_fd);
     if (ret == -1) {
         err = errno;
         DEBUG(SSSDBG_CRIT_FAILURE,
@@ -748,7 +749,7 @@ errno_t exec_child(TALLOC_CTX *mem_ctx,
     }
 
     close(pipefd_from_child[0]);
-    ret = dup2(pipefd_from_child[1], STDOUT_FILENO);
+    ret = dup2(pipefd_from_child[1], child_out_fd);
     if (ret == -1) {
         err = errno;
         DEBUG(SSSDBG_CRIT_FAILURE,
@@ -770,6 +771,15 @@ errno_t exec_child(TALLOC_CTX *mem_ctx,
     return err;
 }
 
+errno_t exec_child(TALLOC_CTX *mem_ctx,
+                   int *pipefd_to_child, int *pipefd_from_child,
+                   const char *binary, int debug_fd)
+{
+    return exec_child_ex(mem_ctx, pipefd_to_child, pipefd_from_child,
+                         binary, debug_fd, NULL,
+                         STDIN_FILENO, STDOUT_FILENO);
+}
+
 void child_cleanup(int readfd, int writefd)
 {
     int ret;
diff --git a/src/util/child_common.h b/src/util/child_common.h
index e659388ece3677b7746c159d7de3e86171bb4146..369de71a13449beb185e5bc682c8871625fe6027 100644
--- a/src/util/child_common.h
+++ b/src/util/child_common.h
@@ -112,10 +112,18 @@ void child_sig_handler(struct tevent_context *ev,
                        int count, void *__siginfo, void *pvt);
 
 /* Never returns EOK, ether returns an error, or doesn't return on success */
+errno_t exec_child_ex(TALLOC_CTX *mem_ctx,
+                      int *pipefd_to_child, int *pipefd_from_child,
+                      const char *binary, int debug_fd,
+                      const char *extra_argv[],
+                      int child_in_fd, int child_out_fd);
+
+/* Same as exec_child_ex() except child_in_fd is set to STDIN_FILENO and
+ * child_out_fd is set to STDOUT_FILENO and extra_argv is always NULL.
+ */
 errno_t exec_child(TALLOC_CTX *mem_ctx,
                    int *pipefd_to_child, int *pipefd_from_child,
-                   const char *binary, int debug_fd,
-                   const char *extra_argv[]);
+                   const char *binary, int debug_fd);
 
 void child_cleanup(int readfd, int writefd);
 
-- 
1.9.3

-------------- next part --------------
>From 59742122568c3bcfe0f1f6b4f0389ebdffe662f0 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Wed, 7 Jan 2015 10:42:09 +0100
Subject: [PATCH 3/3] GPO: Don't use stdout for output in gpo_child

Resolves:
    https://fedorahosted.org/sssd/ticket/2544

Use a dedicated fd instead to work around
https://bugzilla.samba.org/show_bug.cgi?id=11036
---
 src/providers/ad/ad_gpo.c       | 7 ++++---
 src/providers/ad/ad_gpo.h       | 4 ++++
 src/providers/ad/ad_gpo_child.c | 7 ++++---
 3 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/src/providers/ad/ad_gpo.c b/src/providers/ad/ad_gpo.c
index 4f10a8818843dfa52faa982684d9d2737c72ba59..375ef1d8a7df13911831a55fed5d5a425daaa996 100644
--- a/src/providers/ad/ad_gpo.c
+++ b/src/providers/ad/ad_gpo.c
@@ -3961,9 +3961,10 @@ gpo_fork_child(struct tevent_req *req)
     pid = fork();
 
     if (pid == 0) { /* child */
-        err = exec_child(state,
-                         pipefd_to_child, pipefd_from_child,
-                         GPO_CHILD, gpo_child_debug_fd);
+        err = exec_child_ex(state,
+                            pipefd_to_child, pipefd_from_child,
+                            GPO_CHILD, gpo_child_debug_fd, NULL,
+                            STDIN_FILENO, AD_GPO_CHILD_OUT_FILENO);
         DEBUG(SSSDBG_CRIT_FAILURE, "Could not exec gpo_child: [%d][%s].\n",
               err, strerror(err));
         return err;
diff --git a/src/providers/ad/ad_gpo.h b/src/providers/ad/ad_gpo.h
index 77051aadaed7858cce808b5c6c51f0be515d9800..9fd590a2b262b66c1efd493d8736774bdfa61b40 100644
--- a/src/providers/ad/ad_gpo.h
+++ b/src/providers/ad/ad_gpo.h
@@ -23,6 +23,10 @@
 #ifndef AD_GPO_H_
 #define AD_GPO_H_
 
+#include "providers/ad/ad_access.h"
+
+#define AD_GPO_CHILD_OUT_FILENO 3
+
 /*
  * This pair of functions provides client-side GPO processing.
  *
diff --git a/src/providers/ad/ad_gpo_child.c b/src/providers/ad/ad_gpo_child.c
index aa0ab0dc351c7a29c1526d810d5b3cbae5562c5a..1254b69cd9581598d2d63f186bf96557132f0d1a 100644
--- a/src/providers/ad/ad_gpo_child.c
+++ b/src/providers/ad/ad_gpo_child.c
@@ -33,6 +33,7 @@
 #include "util/util.h"
 #include "util/child_common.h"
 #include "providers/dp_backend.h"
+#include "providers/ad/ad_gpo.h"
 #include "sss_cli.h"
 
 #define SMB_BUFFER_SIZE 65536
@@ -778,7 +779,7 @@ main(int argc, const char *argv[])
 
     errno = 0;
 
-    written = sss_atomic_write_s(STDOUT_FILENO, resp->buf, resp->size);
+    written = sss_atomic_write_s(AD_GPO_CHILD_OUT_FILENO, resp->buf, resp->size);
     if (written == -1) {
         ret = errno;
         DEBUG(SSSDBG_CRIT_FAILURE, "write failed [%d][%s].\n", ret,
@@ -793,13 +794,13 @@ main(int argc, const char *argv[])
     }
 
     DEBUG(SSSDBG_TRACE_FUNC, "gpo_child completed successfully\n");
-    close(STDOUT_FILENO);
+    close(AD_GPO_CHILD_OUT_FILENO);
     talloc_free(main_ctx);
     return EXIT_SUCCESS;
 
 fail:
     DEBUG(SSSDBG_CRIT_FAILURE, "gpo_child failed!\n");
-    close(STDOUT_FILENO);
+    close(AD_GPO_CHILD_OUT_FILENO);
     talloc_free(main_ctx);
     return EXIT_FAILURE;
 }
-- 
1.9.3



More information about the sssd-devel mailing list