[SSSD] [PATCH] krb5_child: set debugging sooner

Jakub Hrozek jhrozek at redhat.com
Tue Mar 6 16:36:27 UTC 2012


On Mon, Mar 05, 2012 at 08:42:42AM -0500, Stephen Gallagher wrote:
> On Mon, 2012-03-05 at 13:08 +0100, Jakub Hrozek wrote:
> > On Fri, Mar 02, 2012 at 09:39:18AM -0500, Stephen Gallagher wrote:
> > > On Fri, 2012-03-02 at 15:25 +0100, Jakub Hrozek wrote:
> > > > On Fri, Mar 02, 2012 at 08:25:05AM -0500, Stephen Gallagher wrote:
> > > > > On Fri, 2012-03-02 at 14:16 +0100, Jakub Hrozek wrote:
> > > > > > Both krb5_child and ldap_child would emit a "child started" message and
> > > > > > only after that set up debugging to file. This might confuse users,
> > > > > > because unless there is an error, the krb5_child.log might actually be
> > > > > > empty.
> > > > > 
> > > > > Nack. "[sssd[krb5_child[%d]]]", getpid() isn't dependent on anything
> > > > > that you don't know at this point. Just talloc_asprintf() it on NULL and
> > > > > then steal it onto pd later.
> > > > > 
> > > > > Also, please add NULL-checks for the talloc_asprintf() calls. If they
> > > > > return NULL, just assign a static string "[sssd[ldap_child]]" or
> > > > > "[sssd[krb5_child]]" without the PID.
> > > > > 
> > > > 
> > > > OK, new patch is attached. We won't be able to free debug_prg_name if
> > > > talloc_zero fails later, but that's not a big deal, the child process is
> > > > not a long-running one.
> > > > 
> > > 
> > > Hmm, that's a good point. Coverity and valgrind will likely complain
> > > about the leak as well. On further thought, it's probably alright to
> > > just fail the krb5_child if that asprintf doesn't work, because if we're
> > > in that serious of an OOM situation, chances are high that other, more
> > > important things will be failing anyway.
> > > 
> > > So let's do that. Sorry for the repeated revisions.
> > > 
> > > > > > 
> > > > > > I'm thinking we might also add a couple of "tracing" DEBUG messages so
> > > > > > that we can follow the flow in the subprocess more easily.
> > > > > 
> > > > > Please open an RFE.
> > > > 
> > > > https://fedorahosted.org/sssd/ticket/1225
> > > 
> > > Thanks.
> > 
> > A new patch is attached.
> 
> Nack. With these new patches, you may jump to "done" before main_ctx has
> been set. You need to initialize main_ctx to NULL.
> 
> Also, I'd prefer if we went to "done" instead of calling _exit(-1); if
> the allocation of main_ctx fails.

Thank you, yet another patch attached.
-------------- next part --------------
From 689b710f14b4c44b3faba9a44eca5ae4855d5a2b Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Thu, 1 Mar 2012 16:41:14 +0100
Subject: [PATCH] krb5_child: set debugging sooner

---
 src/providers/krb5/krb5_child.c |   30 ++++++++++++++++++------------
 src/providers/ldap/ldap_child.c |   28 +++++++++++++++++-----------
 2 files changed, 35 insertions(+), 23 deletions(-)

diff --git a/src/providers/krb5/krb5_child.c b/src/providers/krb5/krb5_child.c
index cc185260ef95c275a97e7f21bddb0580bf6a36a5..6aeb7623f40131e448db000af1f20f9d524d202e 100644
--- a/src/providers/krb5/krb5_child.c
+++ b/src/providers/krb5/krb5_child.c
@@ -1582,7 +1582,7 @@ int main(int argc, const char *argv[])
         POPT_TABLEEND
     };
 
-    /* Set debug level to invalid value so we can deside if -d 0 was used. */
+    /* Set debug level to invalid value so we can decide if -d 0 was used. */
     debug_level = SSSDBG_INVALID;
 
     pc = poptGetContext(argv[0], argc, argv, long_options, 0);
@@ -1600,27 +1600,33 @@ int main(int argc, const char *argv[])
 
     CONVERT_AND_SET_DEBUG_LEVEL(debug_level);
 
-    DEBUG(7, ("krb5_child started.\n"));
-
-    pd = talloc_zero(NULL, struct pam_data);
-    if (pd == NULL) {
-        DEBUG(1, ("malloc failed.\n"));
-        _exit(-1);
+    debug_prg_name = talloc_asprintf(NULL, "[sssd[krb5_child[%d]]]", getpid());
+    if (!debug_prg_name) {
+        DEBUG(SSSDBG_CRIT_FAILURE, ("talloc_asprintf failed.\n"));
+        goto fail;
     }
 
-    debug_prg_name = talloc_asprintf(pd, "[sssd[krb5_child[%d]]]", getpid());
-
     if (debug_fd != -1) {
         ret = set_debug_file_from_fd(debug_fd);
         if (ret != EOK) {
-            DEBUG(1, ("set_debug_file_from_fd failed.\n"));
+            DEBUG(SSSDBG_CRIT_FAILURE, ("set_debug_file_from_fd failed.\n"));
         }
     }
 
+    DEBUG(SSSDBG_TRACE_FUNC, ("krb5_child started.\n"));
+
+    pd = talloc_zero(NULL, struct pam_data);
+    if (pd == NULL) {
+        DEBUG(SSSDBG_CRIT_FAILURE, ("talloc_zero failed.\n"));
+        talloc_free(discard_const(debug_prg_name));
+        goto fail;
+    }
+    talloc_steal(pd, debug_prg_name);
+
     buf = talloc_size(pd, sizeof(uint8_t)*IN_BUF_SIZE);
     if (buf == NULL) {
-        DEBUG(1, ("malloc failed.\n"));
-        _exit(-1);
+        DEBUG(SSSDBG_CRIT_FAILURE, ("malloc failed.\n"));
+        goto fail;
     }
 
     while ((ret = read(STDIN_FILENO, buf + len, IN_BUF_SIZE - len)) != 0) {
diff --git a/src/providers/ldap/ldap_child.c b/src/providers/ldap/ldap_child.c
index 66ceb14e3d73d5af448029ef226bd97001fa008f..025236e5eb3b1a3bec601d5f32d7d035999523e8 100644
--- a/src/providers/ldap/ldap_child.c
+++ b/src/providers/ldap/ldap_child.c
@@ -366,7 +366,7 @@ int main(int argc, const char *argv[])
     int opt;
     int debug_fd = -1;
     poptContext pc;
-    TALLOC_CTX *main_ctx;
+    TALLOC_CTX *main_ctx = NULL;
     uint8_t *buf = NULL;
     ssize_t len = 0;
     const char *ccname = NULL;
@@ -388,7 +388,7 @@ int main(int argc, const char *argv[])
         POPT_TABLEEND
     };
 
-    /* Set debug level to invalid value so we can deside if -d 0 was used. */
+    /* Set debug level to invalid value so we can decide if -d 0 was used. */
     debug_level = SSSDBG_INVALID;
 
     pc = poptGetContext(argv[0], argc, argv, long_options, 0);
@@ -406,23 +406,29 @@ int main(int argc, const char *argv[])
 
     CONVERT_AND_SET_DEBUG_LEVEL(debug_level);
 
-    DEBUG(7, ("ldap_child started.\n"));
-
-    main_ctx = talloc_new(NULL);
-    if (main_ctx == NULL) {
-        DEBUG(1, ("talloc_new failed.\n"));
-        _exit(-1);
+    debug_prg_name = talloc_asprintf(NULL, "[sssd[ldap_child[%d]]]", getpid());
+    if (!debug_prg_name) {
+        DEBUG(SSSDBG_CRIT_FAILURE, ("talloc_asprintf failed.\n"));
+        goto fail;
     }
 
-    debug_prg_name = talloc_asprintf(main_ctx, "[sssd[ldap_child[%d]]]", getpid());
-
     if (debug_fd != -1) {
         ret = set_debug_file_from_fd(debug_fd);
         if (ret != EOK) {
-            DEBUG(1, ("set_debug_file_from_fd failed.\n"));
+            DEBUG(SSSDBG_CRIT_FAILURE, ("set_debug_file_from_fd failed.\n"));
         }
     }
 
+    DEBUG(SSSDBG_TRACE_FUNC, ("ldap_child started.\n"));
+
+    main_ctx = talloc_new(NULL);
+    if (main_ctx == NULL) {
+        DEBUG(SSSDBG_CRIT_FAILURE, ("talloc_new failed.\n"));
+        talloc_free(discard_const(debug_prg_name));
+        goto fail;
+    }
+    talloc_steal(main_ctx, debug_prg_name);
+
     buf = talloc_size(main_ctx, sizeof(uint8_t)*IN_BUF_SIZE);
     if (buf == NULL) {
         DEBUG(1, ("talloc_size failed.\n"));
-- 
1.7.7.6



More information about the sssd-devel mailing list