[SSSD] [PATCH] Suppress warning maybe-uninitialized

Lukas Slebodnik lslebodn at redhat.com
Tue Oct 7 21:57:34 UTC 2014


On (03/10/14 09:40), Simo Sorce wrote:
>----- Original Message -----
>> From: "Lukas Slebodnik" <lslebodn at redhat.com>
>> To: "Simo Sorce" <simo at redhat.com>
>> Cc: sssd-devel at lists.fedorahosted.org
>> Sent: Thursday, October 2, 2014 4:20:56 PM
>> Subject: Re: [SSSD] [PATCH] Suppress warning maybe-uninitialized
>> 
>> On (02/10/14 15:12), Simo Sorce wrote:
>> >On Thu, 2 Oct 2014 19:33:02 +0200
>> >Lukas Slebodnik <lslebodn at redhat.com> wrote:
>> >
>> >> ehlo,
>> >> 
>> >> There is a warning with some version of gcc (4.9.1, 4.4.7).
>> >> Warning is not visible with gcc 4.8
>> >> In my opinion, variable "a" should be initialized every time if
>> >> "maps" is not NULL.
>> >
>> >I agree that "a" should be intialized every time, in fact I do not
>> >agree with your patch where you set it to be a volatile, it is not.
>> >
>> >The main issue here is that the name and way this variable is used is
>> >confusing.
>> >
>> >A better way would be to define a locally in the first loop, and in the
>> >function define a better name initialized to a negative value.
>> >
>> >the name should probably be called something like base_attr_idx and
>> >intialized as:
>> >
>> >int base_attr_idx = 0;
>> >
>> >and if the first part of the code finds the base_attr it is looking for
>> >it would simply assing a to it (note we do away with "a" completely).
>> >
>> >if (map) {
>> >    for (i = 1; i < attrs_num; i++) {
>> >       ...
>> >    }
>> >    if (i < attrs_num) {
>> >        store = true;
>> >        base_attr_idx = i;
>> >        ...
>> >    }
>> >}
>> >
>> >then the second for loop where you get the error would be change to:
>> >for (ai = base_attr_idx; ai > 0 && ai < attrs_num; ai++) {
>> >
>> I suppose you meant similar patch
>> diff --git a/src/providers/ldap/sdap.c b/src/providers/ldap/sdap.c
>> index 7034b11..6e8b16e 100644
>> --- a/src/providers/ldap/sdap.c
>> +++ b/src/providers/ldap/sdap.c
>> @@ -305,6 +305,7 @@ int sdap_parse_entry(TALLOC_CTX *memctx,
>>      char *str;
>>      int lerrno;
>>      int a, i, ret, ai;
>> +    int base_attr_idx;
>>      const char *name;
>>      bool store;
>>      bool base64;
>> @@ -412,6 +413,7 @@ int sdap_parse_entry(TALLOC_CTX *memctx,
>>              /* interesting attr */
>>              if (a < attrs_num) {
>>                  store = true;
>> +                base_attr_idx = a;
>>                  name = map[a].sys_name;
>>                  if (strcmp(name, SYSDB_SSH_PUBKEY) == 0) {
>>                      base64 = true;
>> @@ -478,7 +480,7 @@ int sdap_parse_entry(TALLOC_CTX *memctx,
>>                           * attrs in case there is a map. Find all that match
>>                           * and copy the value
>>                           */
>> -                        for (ai = a; ai < attrs_num; ai++) {
>> +                        for (ai = base_attr_idx; ai < attrs_num; ai++) {
>>                              /* check if this attr is valid with the chosen
>>                               * schema */
>>                              if (!map[ai].name) continue;
>> 
>> 
>> It didn't help.
>> src/providers/ldap/sdap.c: In function 'sdap_parse_entry':
>> src/providers/ldap/sdap.c:308:9: error: 'base_attr_idx' may be used
>> uninitialized in this function [-Werror=maybe-uninitialized]
>>      int base_attr_idx;
>>          ^
>> cc1: all warnings being treated as errors
>> (BTW: There is no difference between using a and base_attr_idx)
>> 
>> Any other better solution is welcomed.
>
>You did not follow my advice.
>There were 2 fundamental changes in my proposal
>
>1. declare and initialize the new variable to 0
>int base_attr_idx = 0;
>
>2. in the second for loop add ai > 0 to the condition, otherwise you'll
>run through even if base_attr_idx was never intentionally set.
>
>With these 2 changes you won't get the warning and the behaviour will be correct.
>
>Additionally, if you want, you can completely drop the 'a' variable and use 'i'
>everywhere instead.
>
Updated version of patch is attached.

LS
-------------- next part --------------
>From 6e956c191b180c350fd3104637b87a87c3ba4007 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lslebodn at redhat.com>
Date: Tue, 7 Oct 2014 23:54:51 +0200
Subject: [PATCH] SDAP: Suppress warning maybe-uninitialized

There is a warning with some version of gcc (4.9.1, 4.4.7).
In my opinion, variable "a" should be initialized every time if
"maps" is not NULL.

src/providers/ldap/sdap.c: In function 'sdap_parse_entry':
src/providers/ldap/sdap.c:481:56: warning: 'a' may be used uninitialized in
                                  this function [-Wmaybe-uninitialized]
                         for (ai = a; ai < attrs_num; ai++) {
                                                        ^
src/providers/ldap/sdap.c:307:9: note: 'a' was declared here
     int a, i, ret, ai;
         ^
  CCLD     libsss_ldap_common.la
---
 src/providers/ldap/sdap.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/src/providers/ldap/sdap.c b/src/providers/ldap/sdap.c
index 7034b11ed8e3aabdcff94462079484dd3ca53ef9..24f6df36c4adfd2f416aef4afcd8ae203137d015 100644
--- a/src/providers/ldap/sdap.c
+++ b/src/providers/ldap/sdap.c
@@ -304,7 +304,8 @@ int sdap_parse_entry(TALLOC_CTX *memctx,
     struct ldb_val v;
     char *str;
     int lerrno;
-    int a, i, ret, ai;
+    int i, ret, ai;
+    int base_attr_idx = 0;
     const char *name;
     bool store;
     bool base64;
@@ -403,16 +404,17 @@ int sdap_parse_entry(TALLOC_CTX *memctx,
         }
 
         if (map) {
-            for (a = 1; a < attrs_num; a++) {
+            for (i = 1; i < attrs_num; i++) {
                 /* check if this attr is valid with the chosen schema */
-                if (!map[a].name) continue;
+                if (!map[i].name) continue;
                 /* check if it is an attr we are interested in */
-                if (strcasecmp(base_attr, map[a].name) == 0) break;
+                if (strcasecmp(base_attr, map[i].name) == 0) break;
             }
             /* interesting attr */
-            if (a < attrs_num) {
+            if (i < attrs_num) {
                 store = true;
-                name = map[a].sys_name;
+                name = map[i].sys_name;
+                base_attr_idx = 1;
                 if (strcmp(name, SYSDB_SSH_PUBKEY) == 0) {
                     base64 = true;
                 }
@@ -478,7 +480,7 @@ int sdap_parse_entry(TALLOC_CTX *memctx,
                          * attrs in case there is a map. Find all that match
                          * and copy the value
                          */
-                        for (ai = a; ai < attrs_num; ai++) {
+                        for (ai = base_attr_idx; ai < attrs_num; ai++) {
                             /* check if this attr is valid with the chosen
                              * schema */
                             if (!map[ai].name) continue;
-- 
2.1.0



More information about the sssd-devel mailing list