[SSSD] [PATCH] Fixing critical format string issues.

Jakub Hrozek jhrozek at redhat.com
Sat May 18 16:55:29 UTC 2013


On Fri, May 17, 2013 at 07:29:01PM +0200, Lukas Slebodnik wrote:
> On (17/05/13 18:50), Lukas Slebodnik wrote:
> >ehlo,
> >
> >This week Ondra fix segfault, which was caused by missing argument
> >in debug message.
> >>             ("nsupdate_get_addrs_done failed: [%d]: [%s]\n",
> >>               sss_strerror(ret)));
> >
> >I was working on another ticket and I spotted similar bug.
> >So I decided to enable printf format checking in debug_fn.
> >There was a huge number of warnings. Attached patch fixes the most critical.
> >--missing argument(s).
> >--format '%s', but argument is integer.
> >--wrong format string, example: '%\n'
> >
> >There are another 150 warnings, but they aren't critical.
> >for example:
> >format '%d' expects argument of type 'int', but argument 2 has type 'size_t'
> >

Hi,

this is a really good patch that fixes many potentially bad issues.

I think we should still fix these eventually, for instance for size_t,
we should use the %z conversion.

> I forgot to wrote in the first mail:
> Remaining warnings are not critical, but they are. So I don't want to pollute
> output from compiler with warnings, therefore patch did not enable
> printf format checking in debug_fn.
> 

What if we enabled the warnings only if SSSD was compiled with a certain
preprocessor directive? Then the developers would see if they introduce
any new warning, but compiling the "production" code that runs with
debug_level=0 anyway would not produce any warnings.

About the patch - almost ack. Please squash the attached patch to fix
some debug levels and I'll ack.
-------------- next part --------------
>From d618a1aa5d6472e5a8f1c22ab9c57ed3417730b2 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Sat, 18 May 2013 18:50:54 +0200
Subject: [PATCH] Fix debug level in string issues

---
 src/providers/ldap/sdap_async_groups.c | 3 ++-
 src/responder/pam/pamsrv_dp.c          | 7 ++++---
 src/sbus/sssd_dbus_server.c            | 5 +++--
 3 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/src/providers/ldap/sdap_async_groups.c b/src/providers/ldap/sdap_async_groups.c
index f843086a8f3078300400d5b23d62d1b04eee7b2f..dc7fba42678e50f367704e03b1a5d476a61f6c56 100644
--- a/src/providers/ldap/sdap_async_groups.c
+++ b/src/providers/ldap/sdap_async_groups.c
@@ -2052,7 +2052,8 @@ static errno_t sdap_nested_group_populate_users(TALLOC_CTX *mem_ctx,
             ret = EINVAL;
         }
         if (ret != EOK) {
-            DEBUG(1, ("User entry %d has no originalDN attribute\n", i));
+            DEBUG(SSSDBG_CRIT_FAILURE,
+                  ("User entry %d has no originalDN attribute\n", i));
             goto done;
         }
         original_dn = (const char *) el->values[0].data;
diff --git a/src/responder/pam/pamsrv_dp.c b/src/responder/pam/pamsrv_dp.c
index 950e5d5c53753c710867679ed07d13975254f7a9..d5a52446fe74d1f2cd883a9692c3df69185b070a 100644
--- a/src/responder/pam/pamsrv_dp.c
+++ b/src/responder/pam/pamsrv_dp.c
@@ -117,9 +117,10 @@ int pam_dp_send_req(struct pam_auth_req *preq, int timeout)
     res = sss_dp_get_domain_conn(preq->cctx->rctx,
                                  preq->domain->conn_name, &be_conn);
     if (res != EOK) {
-        DEBUG(1, ("The Data Provider connection for %s is not available!"
-                  " This maybe a bug, it shouldn't happen!\n",
-                  preq->domain->conn_name));
+        DEBUG(SSSDBG_CRIT_FAILURE,
+              ("The Data Provider connection for %s is not available!"
+               " This maybe a bug, it shouldn't happen!\n",
+               preq->domain->conn_name));
         return EIO;
     }
 
diff --git a/src/sbus/sssd_dbus_server.c b/src/sbus/sssd_dbus_server.c
index 3e5a1e42b927babbc2e4256833264399a62c9dab..3ada25b6127ba23db8f03e7d02c16b43cd542056 100644
--- a/src/sbus/sssd_dbus_server.c
+++ b/src/sbus/sssd_dbus_server.c
@@ -165,8 +165,9 @@ remove_socket_symlink(const char *symlink_name)
     ret = unlink(symlink_name);
     if (ret != 0) {
         ret = errno;
-        DEBUG(2, ("unlink failed to remove [%s] [%d]: %s\n",
-                   symlink_name, ret, strerror(ret)));
+        DEBUG(SSSDBG_CRIT_FAILURE,
+              ("unlink failed to remove [%s] [%d]: %s\n",
+               symlink_name, ret, strerror(ret)));
         return ret;
     }
 
-- 
1.8.2.1



More information about the sssd-devel mailing list