[SSSD] [PATCH] Assorted fixes found by the clang static analyzer

Jakub Hrozek jhrozek at redhat.com
Wed May 2 21:05:55 UTC 2012


On Wed, May 02, 2012 at 08:12:06AM -0400, Stephen Gallagher wrote:
> On Thu, 2012-04-26 at 13:51 +0200, Jakub Hrozek wrote:
> > I scanned the SSSD source code with the clang static analyzer and found
> > a number of issues. Patches are attached -- I think that most of them
> > are OK to just include in master. Patches #1, #13 and #16 may be
> > something that we want to include sooner.
> > 
> > Because the clang analyzer is freely available in Fedora, I think it would
> > make sense to run a scan at least before a release.
> > 
> > Developers can also run the clang tool themselves - I'll add info how
> > into our Developers page.
> 
> 
> Patch 0004: Nack
> There is no NULL-termination guarantee here. We only initialize pid_str
> with the first element being NULL.

This is the pid_str initializer:
    char pid_str[MAX_PID_LENGTH] = {'\0'};

I think that this construct initializes the *whole* array with zeroes,
because if there is an initializer but the number of initializers is less
than the number of array elements, the rest of the array elements are set
to 0.

> 
> 
> Patch 0010: Nack
> If we're reporting ret (which is an errno_t value) I'd prefer if the
> DEBUG messages included the strerror() conversion, since it's much
> easier to read.

Fixed

> 
> Patch 0014: Nack
> This is not the only case in ipa_get_autofs_options(), but returning
> here leaks tmp_ctx and anything still attached to it. Please fix all
> such cases here.
> 

Fixed

> 
> Patch 0018: Nack
> A tevent_req *_send() function should only return NULL if the
> tevent_req_create() fails. Otherwise, all other failures should be
> reported with tevent_req_error() and tevent_req_post().
> 

Fixed, but frankly, there's tens of _send() functions in SSSD that just
return NULL on any failure.
-------------- next part --------------
>From 79ace776ffbe273fda17c173ef1e33f21a08fcfb Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Wed, 25 Apr 2012 16:45:06 +0200
Subject: [PATCH 1/4] SSS_DEBUGLEVEL: silence analyzer warnings

Errno was returned instead of ret.

The other hunk removes return code from fread - it is not needed, the
NULL termination of the string is ensured by initializing the buffer.
---
 src/tools/sss_debuglevel.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/tools/sss_debuglevel.c b/src/tools/sss_debuglevel.c
index 83be144422eed47ef013b9d5bac837f10a6a9b6e..623cea2acd9677b5a6b954f71db981c64ed316e9 100644
--- a/src/tools/sss_debuglevel.c
+++ b/src/tools/sss_debuglevel.c
@@ -218,7 +218,7 @@ errno_t send_sighup()
         ret = errno;
         DEBUG(SSSDBG_CRIT_FAILURE, ("Could not send SIGHUP to process %d: %s\n",
               pid, strerror(errno)));
-        return errno;
+        return ret;
     }
 
     return EOK;
@@ -333,7 +333,7 @@ errno_t get_sssd_pid(pid_t *out_pid)
         goto done;
     }
 
-    ret = fread(pid_str, sizeof(char), MAX_PID_LENGTH * sizeof(char), pid_file);
+    fread(pid_str, sizeof(char), MAX_PID_LENGTH * sizeof(char), pid_file);
     if (!feof(pid_file)) {
         /* eof not reached */
         ret = ferror(pid_file);
-- 
1.7.7.6

-------------- next part --------------
>From 0f064a706a7a0b202319c2663801524d316ab0c2 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Wed, 25 Apr 2012 17:15:21 +0200
Subject: [PATCH 2/4] PROXY: return correct return codes

We were reporting on the value of "status" instead of "ret'. We also
didn't set ret to EOK in cases group contained no members.
---
 src/providers/proxy/proxy_id.c |   16 +++++++++-------
 1 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/src/providers/proxy/proxy_id.c b/src/providers/proxy/proxy_id.c
index e1325b74d3f97cde38e182ec659f50aed93e185a..8a8c7ca80d1b24e53c3d55d06564e719a069642a 100644
--- a/src/providers/proxy/proxy_id.c
+++ b/src/providers/proxy/proxy_id.c
@@ -143,8 +143,8 @@ done:
     talloc_zfree(tmpctx);
     if (ret) {
         DEBUG(SSSDBG_OP_FAILURE,
-              ("proxy -> getpwnam_r failed for '%s' <%d>\n",
-               name, status));
+              ("proxy -> getpwnam_r failed for '%s' <%d>: %s\n",
+               name, ret, strerror(ret)));
     }
     return ret;
 }
@@ -334,7 +334,8 @@ done:
     talloc_zfree(tmpctx);
     if (ret) {
         DEBUG(SSSDBG_CRIT_FAILURE,
-              ("proxy -> getpwuid_r failed for '%d' <%d>\n", uid, status));
+              ("proxy -> getpwuid_r failed for '%d' <%d>: %s\n",
+               uid, ret, strerror(ret)));
     }
     return ret;
 }
@@ -656,6 +657,7 @@ static errno_t proxy_process_missing_users(struct sysdb_ctx *sysdb,
         }
     }
 
+    ret = EOK;
 done:
     talloc_free(tmp_ctx);
     return ret;
@@ -848,8 +850,8 @@ done:
     talloc_zfree(tmpctx);
     if (ret) {
         DEBUG(SSSDBG_OP_FAILURE,
-              ("proxy -> getgrnam_r failed for '%s' <%d>\n",
-              name, status));
+              ("proxy -> getgrnam_r failed for '%s' <%d>: %s\n",
+              name, ret, strerror(ret)));
     }
     return ret;
 }
@@ -917,8 +919,8 @@ done:
     talloc_zfree(tmpctx);
     if (ret) {
         DEBUG(SSSDBG_OP_FAILURE,
-              ("proxy -> getgrgid_r failed for '%d' <%d>\n",
-               gid, status));
+              ("proxy -> getgrgid_r failed for '%d' <%d>: %s\n",
+               gid, ret, strerror(ret)));
     }
     return ret;
 }
-- 
1.7.7.6

-------------- next part --------------
>From 21825b12a500226a68127a98a1da7d9abceab987 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Thu, 26 Apr 2012 11:21:59 +0200
Subject: [PATCH 3/4] IPA: Check return values

---
 src/providers/ipa/ipa_common.c |    8 ++++++--
 src/providers/ipa/ipa_hosts.c  |    6 ++++++
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/src/providers/ipa/ipa_common.c b/src/providers/ipa/ipa_common.c
index 225c9786c47ffe4895ad24a77b1cb23550839e48..262a9bccc1c9f113202a60f5ba551018d3d07f7f 100644
--- a/src/providers/ipa/ipa_common.c
+++ b/src/providers/ipa/ipa_common.c
@@ -916,6 +916,10 @@ int ipa_get_autofs_options(struct ipa_options *ipa_opts,
     ret = sdap_parse_search_base(ipa_opts->id, ipa_opts->id->basic,
                                  SDAP_AUTOFS_SEARCH_BASE,
                                  &ipa_opts->id->autofs_search_bases);
+    if (ret != EOK && ret != ENOENT) {
+        DEBUG(SSSDBG_OP_FAILURE, ("Could not parse autofs search base\n"));
+        goto done;
+    }
 
     ret = sdap_get_map(ipa_opts->id, cdb, conf_path,
                        ipa_autofs_mobject_map,
@@ -924,7 +928,7 @@ int ipa_get_autofs_options(struct ipa_options *ipa_opts,
     if (ret != EOK) {
         DEBUG(SSSDBG_OP_FAILURE,
               ("Could not get autofs map object attribute map\n"));
-        return ret;
+        goto done;
     }
 
     ret = sdap_get_map(ipa_opts->id, cdb, conf_path,
@@ -934,7 +938,7 @@ int ipa_get_autofs_options(struct ipa_options *ipa_opts,
     if (ret != EOK) {
         DEBUG(SSSDBG_OP_FAILURE,
               ("Could not get autofs entry object attribute map\n"));
-        return ret;
+        goto done;
     }
 
     *_opts = ipa_opts->id;
diff --git a/src/providers/ipa/ipa_hosts.c b/src/providers/ipa/ipa_hosts.c
index 5252ac9b4de33862d5526e23b10fe14df8e5bc8a..300138648f3029a6d94e60ade6e6c00977618f66 100644
--- a/src/providers/ipa/ipa_hosts.c
+++ b/src/providers/ipa/ipa_hosts.c
@@ -332,6 +332,12 @@ ipa_hostgroup_info_done(struct tevent_req *subreq)
                                     &hostgroup_count,
                                     &hostgroups);
         talloc_zfree(subreq);
+        if (ret != EOK) {
+            DEBUG(SSSDBG_OP_FAILURE,
+                  ("sdap_get_generic_recv failed: [%d]\n", ret));
+            tevent_req_error(req, ret);
+            return;
+        }
 
         /* Merge the two arrays */
         if (hostgroup_count > 0) {
-- 
1.7.7.6

-------------- next part --------------
>From 0f3ca2b6dc5efb64a9b6ee4eac7fb9ad6d205f5c Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Thu, 26 Apr 2012 13:06:26 +0200
Subject: [PATCH 4/4] AUTOFS: remove unused assignments

Also changes setautomntent_send so that is only return NULL in case the
tevent_req creation fails.
---
 src/responder/autofs/autofssrv_cmd.c |   13 +++++++++----
 src/sss_client/autofs/sss_autofs.c   |    1 -
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/src/responder/autofs/autofssrv_cmd.c b/src/responder/autofs/autofssrv_cmd.c
index ebf68b84a3d73d2b36c8a203c0be57cb50fbbcd2..7064591abc83e4f54a6c63779510893145b1fdcb 100644
--- a/src/responder/autofs/autofssrv_cmd.c
+++ b/src/responder/autofs/autofssrv_cmd.c
@@ -361,13 +361,14 @@ setautomntent_send(TALLOC_CTX *mem_ctx,
     if (!req) {
         DEBUG(SSSDBG_FATAL_FAILURE,
               ("Could not create tevent request for setautomntent\n"));
-        goto fail;
+        return NULL;
     }
     state->cmdctx = cmdctx;
 
     dctx = talloc_zero(state, struct autofs_dom_ctx);
     if (!dctx) {
         DEBUG(SSSDBG_FATAL_FAILURE, ("Out of memory\n"));
+        ret = ENOMEM;
         goto fail;
     }
     dctx->cmd_ctx = state->cmdctx;
@@ -388,11 +389,13 @@ setautomntent_send(TALLOC_CTX *mem_ctx,
     if (domname) {
         dctx->domain = responder_get_domain(dctx, client->rctx, domname);
         if (!dctx->domain) {
+            ret = EINVAL;
             goto fail;
         }
 
         client->automntmap_name = talloc_strdup(client, rawname);
         if (!client->automntmap_name) {
+            ret = ENOMEM;
             goto fail;
         }
     } else {
@@ -402,6 +405,7 @@ setautomntent_send(TALLOC_CTX *mem_ctx,
 
         client->automntmap_name = talloc_strdup(client, state->mapname);
         if (!client->automntmap_name) {
+            ret = ENOMEM;
             goto fail;
         }
     }
@@ -504,6 +508,7 @@ setautomntent_send(TALLOC_CTX *mem_ctx,
         } else if (ret != EOK) {
             DEBUG(SSSDBG_CRIT_FAILURE, ("Could not get data from cache\n"));
             talloc_free(state->map);
+            ret = ENOMEM;
             goto fail;
         }
 
@@ -520,8 +525,9 @@ setautomntent_send(TALLOC_CTX *mem_ctx,
     return req;
 
 fail:
-    talloc_free(req);
-    return NULL;
+    tevent_req_error(req, ret);
+    tevent_req_post(req, actx->rctx->ev);
+    return req;
 }
 
 static errno_t
@@ -585,7 +591,6 @@ lookup_automntmap_step(struct setautomntent_lookup_ctx *lookup_ctx)
                 }
                 else break;
             }
-            ret = EOK;
         }
 
         ret = get_autofs_map(lookup_ctx->actx, lookup_ctx->mapname, &map);
diff --git a/src/sss_client/autofs/sss_autofs.c b/src/sss_client/autofs/sss_autofs.c
index 0125e6eece3bfcc69029e6a1e300159778eb72f3..e87ef4fd22d7ea577696866c00bbbb066bee8274 100644
--- a/src/sss_client/autofs/sss_autofs.c
+++ b/src/sss_client/autofs/sss_autofs.c
@@ -281,7 +281,6 @@ _sss_getautomntent_r(char **key, char **value, void *context)
     }
     /* Don't try to handle any error codes, just go to the responder again */
 
-    ret = 0;
     data_len = sizeof(uint32_t) +            /* mapname len */
                name_len + 1 +                /* mapname\0   */
                sizeof(uint32_t) +            /* index into the map */
-- 
1.7.7.6



More information about the sssd-devel mailing list