[SSSD] [PATCHES] Fix more assorted bugs found by Coverity

Sumit Bose sbose at redhat.com
Wed Dec 15 09:53:22 UTC 2010


On Tue, Dec 14, 2010 at 05:13:54PM -0500, Stephen Gallagher wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 12/14/2010 04:55 PM, Sumit Bose wrote:
> > Hi,
> > 
> > here are some more fixes for issues found by Coverity. Please have a
> > careful look on the patch for ticket #727. I think the patch will fix
> > the issue, but I'm not sure if some other kind of fix is needed here.
> 
> 
> Patch 0001: Ack
> 
> Patch 0002: Ack
> 
> Patch 0003: Ack
> 
> Patch 0004: Ack
> 
> Patch 0005: Ack
> 
> Patch 0006: Ack
> 
> Patch 0007: Ack
> 
> Patch 0008: Nack
> If we have no domains with this netgroup, we want to allocate a netgr
> object here and set it to ready and entries to NULL. Otherwise, it won't
> save it in the in-memory cache and we'll keep going back to the on-disc
> cache all the time.
> 
> Patch 0009: Ack
> 
> Patch 0010: Ack
> 
> Patch 0011: Ack
> 
> 
> 
> Pushed all but 0008 to master.

Thanks, new version is attached. I hope you don't mind, but I've fixed
and indentation issue in this patch, too. I you prefer I can send this
in a separate patch.

bye,
Sumit

> 
> - -- 
> Stephen Gallagher
> RHCE 804006346421761
> 
> Delivering value year after year.
> Red Hat ranks #1 in value among software vendors.
> http://www.redhat.com/promo/vendor/
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.11 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
> 
> iEYEARECAAYFAk0H7CIACgkQeiVVYja6o6O3xwCgkPOr81uAcnYfsOHq1I+UY1ju
> E68An0o909HTTUih1XPfTHjg9nLa1Vb+
> =IpFj
> -----END PGP SIGNATURE-----
-------------- next part --------------
From 3dbdd1e476054e6268903cf49871b49885118c31 Mon Sep 17 00:00:00 2001
From: Sumit Bose <sbose at redhat.com>
Date: Wed, 15 Dec 2010 10:47:57 +0100
Subject: [PATCH] Fix uninitialized value error in lookup_netgr_step()

---
 src/responder/nss/nsssrv_netgroup.c |  327 +++++++++++++++++++----------------
 1 files changed, 181 insertions(+), 146 deletions(-)

diff --git a/src/responder/nss/nsssrv_netgroup.c b/src/responder/nss/nsssrv_netgroup.c
index 1ac2608..2c47be9 100644
--- a/src/responder/nss/nsssrv_netgroup.c
+++ b/src/responder/nss/nsssrv_netgroup.c
@@ -55,6 +55,31 @@ static errno_t get_netgroup_entry(struct nss_ctx *nctx,
     return EIO;
 }
 
+static int netgr_hash_remove (TALLOC_CTX *ctx);
+static errno_t set_netgroup_entry(struct nss_ctx *nctx,
+                                  char *name,
+                                  struct getent_ctx *netgr)
+{
+    hash_key_t key;
+    hash_value_t value;
+    int hret;
+
+    /* Add this entry to the hash table */
+    key.type = HASH_KEY_STRING;
+    key.str = name;
+    value.type = HASH_VALUE_PTR;
+    value.ptr = netgr;
+    hret = hash_enter(nctx->netgroups, &key, &value);
+    if (hret != EOK) {
+        DEBUG(0, ("Unable to add hash table entry for [%s]", key.str));
+        DEBUG(4, ("Hash error [%d][%s]", hret, hash_error_string(hret)));
+        return EIO;
+    }
+    talloc_set_destructor((TALLOC_CTX *) netgr, netgr_hash_remove);
+
+    return EOK;
+}
+
 static struct tevent_req *setnetgrent_send(TALLOC_CTX *mem_ctx,
                                            const char *rawname,
                                            struct nss_cmd_ctx *cmdctx);
@@ -135,11 +160,8 @@ static struct tevent_req *setnetgrent_send(TALLOC_CTX *mem_ctx,
     errno_t ret;
     struct tevent_req *req;
     struct setnetgrent_ctx *state;
-    int hret;
     struct nss_dom_ctx *dctx;
     struct setent_step_ctx *step_ctx;
-    hash_key_t key;
-    hash_value_t value;
 
     struct cli_ctx *client = cmdctx->cctx;
     struct nss_ctx *nctx =
@@ -248,19 +270,12 @@ static struct tevent_req *setnetgrent_send(TALLOC_CTX *mem_ctx,
             goto error;
         }
 
-        /* Add this entry to the hash table */
-        key.type = HASH_KEY_STRING;
-        key.str = client->netgr_name;
-        value.type = HASH_VALUE_PTR;
-        value.ptr = state->netgr;
-        hret = hash_enter(nctx->netgroups, &key, &value);
-        if (hret != EOK) {
-            DEBUG(0, ("Unable to add hash table entry for [%s]", key.str));
-            DEBUG(4, ("Hash error [%d][%s]", hret, hash_error_string(hret)));
+        ret = set_netgroup_entry(nctx, client->netgr_name, state->netgr);
+        if (ret != EOK) {
+            DEBUG(1, ("set_netgroup_entry failed.\n"));
             talloc_free(state->netgr);
             goto error;
         }
-        talloc_set_destructor((TALLOC_CTX *)state->netgr, netgr_hash_remove);
 
         /* Perform lookup */
         step_ctx = talloc_zero(state->netgr, struct setent_step_ctx);
@@ -318,154 +333,174 @@ static void setnetgrent_result_timeout(struct tevent_context *ev,
                                        struct tevent_timer *te,
                                        struct timeval current_time,
                                        void *pvt);
+
+/* Set up a lifetime timer for this result object
+ * We don't want this result object to outlive the
+ * entry cache refresh timeout
+ */
+static void set_netgr_lifetime(uint32_t lifetime,
+                               struct setent_step_ctx *step_ctx,
+                               struct getent_ctx *netgr)
+{
+    struct timeval tv;
+    struct tevent_timer *te;
+
+    tv = tevent_timeval_current_ofs(lifetime, 0);
+    te = tevent_add_timer(step_ctx->nctx->rctx->ev,
+                          step_ctx->nctx->gctx, tv,
+                          setnetgrent_result_timeout,
+                          netgr);
+    if (!te) {
+        DEBUG(0, ("Could not set up life timer for setnetgrent result object. "
+                  "Entries may become stale.\n"));
+    }
+}
+
 static errno_t lookup_netgr_step(struct setent_step_ctx *step_ctx)
 {
     errno_t ret;
     struct sss_domain_info *dom = step_ctx->dctx->domain;
     struct getent_ctx *netgr;
     struct sysdb_ctx *sysdb;
-    struct timeval tv;
-    struct tevent_timer *te;
 
     /* Check each domain for this netgroup name */
     while (dom) {
         /* if it is a domainless search, skip domains that require fully
-          * qualified names instead */
-         while (dom && step_ctx->check_next && dom->fqnames) {
-             dom = dom->next;
-         }
-
-         /* No domains left to search */
-         if (!dom) break;
-
-         if (dom != step_ctx->dctx->domain) {
-             /* make sure we reset the check_provider flag when we check
-              * a new domain */
-             step_ctx->dctx->check_provider =
-                     NEED_CHECK_PROVIDER(dom->provider);
-         }
-
-         /* make sure to update the dctx if we changed domain */
-         step_ctx->dctx->domain = dom;
-
-         /* verify this netgroup has not yet been negatively cached */
-         ret = sss_ncache_check_netgr(step_ctx->nctx->ncache,
-                                      step_ctx->nctx->neg_timeout,
-                                      dom->name, step_ctx->name);
-
-         /* if neg cached, return we didn't find it */
-         if (ret == EEXIST) {
-             DEBUG(2, ("Netgroup [%s] does not exist! (negative cache)\n",
-                       step_ctx->name));
-             /* if a multidomain search, try with next */
-             if (step_ctx->check_next) {
-                 dom = dom->next;
-                 continue;
-             }
-         }
-
-         DEBUG(4, ("Requesting info for [%s@%s]\n",
-                   step_ctx->name, dom->name));
-         ret = sysdb_get_ctx_from_list(step_ctx->rctx->db_list, dom, &sysdb);
-         if (ret != EOK) {
-             DEBUG(0, ("Fatal: Sysdb CTX not found for this domain!\n"));
-             return EIO;
-         }
-
-         /* Look up the netgroup in the cache */
-         ret = sysdb_getnetgr(step_ctx->dctx, sysdb, dom,
-                              step_ctx->name,
-                              &step_ctx->dctx->res);
-         if (ret == ENOENT) {
-             /* This netgroup was not found in this domain */
-             if (!step_ctx->dctx->check_provider) {
-                 if (step_ctx->check_next) {
-                     dom = dom->next;
-                     continue;
-                 }
-                 else break;
-             }
-             ret = EOK;
-         }
-
-         if (ret != EOK) {
-             DEBUG(1, ("Failed to make request to our cache!\n"));
-             return EIO;
-         }
-
-         ret = get_netgroup_entry(step_ctx->nctx, step_ctx->name,
-                                  &netgr);
-         if (ret != EOK) {
-             /* Something really bad happened! */
-             DEBUG(0, ("Netgroup entry was lost!\n"));
-             return ret;
-         }
-
-         /* Convert the result to a list of entries */
-         ret = sysdb_netgr_to_entries(netgr, step_ctx->dctx->res,
-                                      &netgr->entries);
-         if (ret == ENOENT) {
-             /* This netgroup was not found in this domain */
-             if (!step_ctx->dctx->check_provider) {
-                 if (step_ctx->check_next) {
-                     dom = dom->next;
-                     continue;
-                 }
-                 else break;
-             }
-             ret = EOK;
-         }
-
-         if (ret != EOK) {
-             DEBUG(1, ("Failed to convert results into entries\n"));
-             return EIO;
-         }
-
-         /* if this is a caching provider (or if we haven't checked the cache
-          * yet) then verify that the cache is uptodate */
-         if (step_ctx->dctx->check_provider) {
-             ret = check_cache(step_ctx->dctx,
-                               step_ctx->nctx,
-                               step_ctx->dctx->res,
-                               SSS_DP_NETGR,
-                               step_ctx->name, 0,
-                               lookup_netgr_dp_callback,
-                               step_ctx);
-             if (ret != EOK) {
-                 /* May return EAGAIN legitimately to indicate that
-                  * we need to reenter the mainloop
-                  */
-                 return ret;
-             }
-         }
-
-         /* Results found */
-         DEBUG(6, ("Returning info for netgroup [%s@%s]\n",
-                   step_ctx->name, dom->name));
-         netgr->ready = true;
-
-         /* Set up a lifetime timer for this result object
-          * We don't want this result object to outlive the
-          * entry cache refresh timeout
-          */
-         tv = tevent_timeval_current_ofs(dom->entry_cache_timeout, 0);
-         te = tevent_add_timer(step_ctx->nctx->rctx->ev,
-                               step_ctx->nctx->gctx, tv,
-                               setnetgrent_result_timeout,
-                               netgr);
-         if (!te) {
-             DEBUG(0, ("Could not set up life timer for setnetgrent result object. "
-                       "Entries may become stale.\n"));
-         }
-
-         return EOK;
+         * qualified names instead */
+        while (dom && step_ctx->check_next && dom->fqnames) {
+            dom = dom->next;
+        }
+
+        /* No domains left to search */
+        if (!dom) break;
+
+        if (dom != step_ctx->dctx->domain) {
+            /* make sure we reset the check_provider flag when we check
+             * a new domain */
+            step_ctx->dctx->check_provider =
+                    NEED_CHECK_PROVIDER(dom->provider);
+        }
+
+        /* make sure to update the dctx if we changed domain */
+        step_ctx->dctx->domain = dom;
+
+        /* verify this netgroup has not yet been negatively cached */
+        ret = sss_ncache_check_netgr(step_ctx->nctx->ncache,
+                                     step_ctx->nctx->neg_timeout,
+                                     dom->name, step_ctx->name);
+
+        /* if neg cached, return we didn't find it */
+        if (ret == EEXIST) {
+            DEBUG(2, ("Netgroup [%s] does not exist! (negative cache)\n",
+                      step_ctx->name));
+            /* if a multidomain search, try with next */
+            if (step_ctx->check_next) {
+                dom = dom->next;
+                continue;
+            }
+        }
+
+        DEBUG(4, ("Requesting info for [%s@%s]\n",
+                  step_ctx->name, dom->name));
+        ret = sysdb_get_ctx_from_list(step_ctx->rctx->db_list, dom, &sysdb);
+        if (ret != EOK) {
+            DEBUG(0, ("Fatal: Sysdb CTX not found for this domain!\n"));
+            return EIO;
+        }
+
+        /* Look up the netgroup in the cache */
+        ret = sysdb_getnetgr(step_ctx->dctx, sysdb, dom,
+                             step_ctx->name,
+                             &step_ctx->dctx->res);
+        if (ret == ENOENT) {
+            /* This netgroup was not found in this domain */
+            if (!step_ctx->dctx->check_provider) {
+                if (step_ctx->check_next) {
+                    dom = dom->next;
+                    continue;
+                }
+                else break;
+            }
+            ret = EOK;
+        }
+
+        if (ret != EOK) {
+            DEBUG(1, ("Failed to make request to our cache!\n"));
+            return EIO;
+        }
+
+        ret = get_netgroup_entry(step_ctx->nctx, step_ctx->name,
+                                 &netgr);
+        if (ret != EOK) {
+            /* Something really bad happened! */
+            DEBUG(0, ("Netgroup entry was lost!\n"));
+            return ret;
+        }
+
+        /* Convert the result to a list of entries */
+        ret = sysdb_netgr_to_entries(netgr, step_ctx->dctx->res,
+                                     &netgr->entries);
+        if (ret == ENOENT) {
+            /* This netgroup was not found in this domain */
+            if (!step_ctx->dctx->check_provider) {
+                if (step_ctx->check_next) {
+                    dom = dom->next;
+                    continue;
+                }
+                else break;
+            }
+            ret = EOK;
+        }
+
+        if (ret != EOK) {
+            DEBUG(1, ("Failed to convert results into entries\n"));
+            return EIO;
+        }
+
+        /* if this is a caching provider (or if we haven't checked the cache
+         * yet) then verify that the cache is uptodate */
+        if (step_ctx->dctx->check_provider) {
+            ret = check_cache(step_ctx->dctx,
+                              step_ctx->nctx,
+                              step_ctx->dctx->res,
+                              SSS_DP_NETGR,
+                              step_ctx->name, 0,
+                              lookup_netgr_dp_callback,
+                              step_ctx);
+            if (ret != EOK) {
+                /* May return EAGAIN legitimately to indicate that
+                 * we need to reenter the mainloop
+                 */
+                return ret;
+            }
+        }
+
+        /* Results found */
+        DEBUG(6, ("Returning info for netgroup [%s@%s]\n",
+                  step_ctx->name, dom->name));
+        netgr->ready = true;
+        set_netgr_lifetime(dom->entry_cache_timeout, step_ctx, netgr);
+        return EOK;
     }
 
     /* If we've gotten here, then no domain contained this netgroup */
     DEBUG(2, ("No matching domain found for [%s], fail!\n",
               step_ctx->name));
-    netgr->ready = true;
-    netgr->entries = NULL;
+
+    netgr = talloc_zero(step_ctx->nctx, struct getent_ctx);
+    if (netgr == NULL) {
+        DEBUG(1, ("talloc_zero failed, ignored.\n"));
+    } else {
+        netgr->ready = true;
+        netgr->entries = NULL;
+
+        ret = set_netgroup_entry(step_ctx->nctx, step_ctx->name, netgr);
+        if (ret != EOK) {
+            DEBUG(1, ("set_netgroup_entry failed, ignored.\n"));
+        }
+        set_netgr_lifetime(dom->entry_cache_timeout, step_ctx, netgr);
+    }
+
     return ENOENT;
 }
 
-- 
1.7.3.2



More information about the sssd-devel mailing list