[SSSD] [PATCHES] check return value

Lukas Slebodnik lslebodn at redhat.com
Tue Aug 26 15:37:09 UTC 2014


On (26/08/14 13:41), Lukas Slebodnik wrote:
>On (10/04/14 15:05), Lukas Slebodnik wrote:
>>On (10/04/14 14:10), Jakub Hrozek wrote:
>>>On Thu, Apr 10, 2014 at 01:44:08PM +0200, Lukas Slebodnik wrote:
>>>> On (10/04/14 12:11), Pavel Reichl wrote:
>>>> >On Thu, 2014-04-10 at 11:11 +0200, Sumit Bose wrote:
>>>> >> On Thu, Apr 10, 2014 at 10:34:10AM +0200, Pavel Reichl wrote:
>>>> >> > On Wed, 2014-04-09 at 21:19 +0200, Sumit Bose wrote:
>>>> >> > > On Wed, Apr 09, 2014 at 07:27:28PM +0200, Pavel Reichl wrote:
>>>> >> > > > On Mon, 2014-04-07 at 18:13 +0200, Sumit Bose wrote:
>>>> >> > > > > On Mon, Apr 07, 2014 at 02:22:04PM +0200, Pavel Reichl wrote:
>>>> >> > > > > > Hello,
>>>> >> > > > > > 
>>>> >> > > > > > I noticed these two warnings in clang.
>>>> >> > > > > > 
>>>> >> > > > > > It would be great if the 2nd patch could be checked by Sumit to make
>>>> >> > > > > > sure that the return value wasn't ignored on purpose.
>>>> >> > > > > 
>>>> >> > > > > yes, I would prefer to ignore errors here. There might be various cases
>>>> >> > > > > were we are not able to resolve a single SID but still can proceed with
>>>> >> > > > > the others.
>>>> >> > > > 
>>>> >> > > > Please see attached patch. Feel free to NACK it, if you think it's more
>>>> >> > > > pain than gain.
>>>> >> > > 
>>>> >> > > Why not do something useful to avoid the compiler warning and print a
>>>> >> > > SSSDBG_TRACE_ALL debug message with the returned error code in the case
>>>> >> > > of an error?
>>>> >> > 
>>>> >> > Hello Sumit, 
>>>> >> > 
>>>> >> > I just took your previous response too literally - "I would prefer to
>>>> >> > ignore errors here" - my bad.  (Hopefully final) patch attached.
>>>> >> 
>>>> >> Thank you. ACK
>>>> >> 
>>>> >> I wonder if the explicit (unsigned int) casts are needed to avoid
>>>> >> warnings or if you are just calling them to be on the safe side?
>>>> >
>>>> >Honestly, I just copy&past them. When I removed them I didn't notice any
>>>> If you copy&past this code then explicit casting is in two places.
>>>> In future, it can in 3, 4, 5 ...
>>>> 
>>>> >relevant clang or gcc warnings. I asked Jakub and he advised me that I
>>>> >could have used formatting macros PRIu16 and PRIu32.
>>>> >
>>>> >I would suggest to leave patch as is for now.
>>>> >
>>>> Is it a problem to send another patch?
>>>> 
>>>> LS
>>>
>>>If you have time to convert the debug messages, then by all means, send
>>>a patch. Maybe this could be a nice intern task? :-)
>What about this patch?
>
>LS

>From b7d33f8ef584cb1e57fb4a286959a9ddf8aed7f8 Mon Sep 17 00:00:00 2001
>From: Lukas Slebodnik <lslebodn at redhat.com>
>Date: Tue, 26 Aug 2014 11:35:23 +0200
>Subject: [PATCH 1/2] SDAP: Fix using of uninitialized variable
>
>When group was posix and id mapping was not enabled then variable gid was
>used uninitialized.
>
Fixing commit description: id mapping *must be* enabled.

updated version attached.

LS
-------------- next part --------------
>From 80881054d0d2c6a6bdf2ca51b1ee8a64b6c2be1b Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lslebodn at redhat.com>
Date: Tue, 26 Aug 2014 11:35:23 +0200
Subject: [PATCH] SDAP: Fix using of uninitialized variable

When group was posix and id mapping was enabled then variable gid was
used uninitialized.

Valgrind error:
 Conditional jump or move depends on uninitialised value(s)
    at 0x13F1A1D7: sdap_nested_group_hash_group (sdap_async_nested_groups.c:279)
    by 0x13F1DAA1: sdap_nested_group_send (sdap_async_nested_groups.c:718)
    by 0x13F1998D: sdap_get_groups_process (sdap_async_groups.c:1847)
    by 0x13F0F9CE: sdap_get_generic_ext_done (sdap_async.c:1467)
    by 0x13F0EE9F: sdap_process_result (sdap_async.c:357)
    by 0x54ABFBE: tevent_common_loop_timer_delay (in /usr/lib64/libtevent.so.0.9.20)
    by 0x54ACFC9: ??? (in /usr/lib64/libtevent.so.0.9.20)
    by 0x54AB6B6: ??? (in /usr/lib64/libtevent.so.0.9.20)
    by 0x54A7F2C: _tevent_loop_once (in /usr/lib64/libtevent.so.0.9.20)
    by 0x54A80CA: tevent_common_loop_wait (in /usr/lib64/libtevent.so.0.9.20)
    by 0x54AB656: ??? (in /usr/lib64/libtevent.so.0.9.20)
    by 0x5283872: server_loop (server.c:587)
---
 src/providers/ldap/sdap_async_nested_groups.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/providers/ldap/sdap_async_nested_groups.c b/src/providers/ldap/sdap_async_nested_groups.c
index 18a811c771206b97c70961f47ca9b3ec57f6593a..b07616a93da9a5b2007a706958c4c69de4471342 100644
--- a/src/providers/ldap/sdap_async_nested_groups.c
+++ b/src/providers/ldap/sdap_async_nested_groups.c
@@ -244,6 +244,7 @@ sdap_nested_group_hash_group(struct sdap_nested_group_ctx *group_ctx,
     int32_t ad_group_type;
     bool posix_group = true;
     bool use_id_mapping;
+    bool can_find_gid;
 
     if (group_ctx->opts->schema_type == SDAP_SCHEMA_AD) {
         ret = sysdb_attrs_get_int32_t(group, SYSDB_GROUP_TYPE, &ad_group_type);
@@ -272,15 +273,17 @@ sdap_nested_group_hash_group(struct sdap_nested_group_ctx *group_ctx,
                                                           group_ctx->domain->name,
                                                           group_ctx->domain->domain_id);
 
-    if (posix_group && !use_id_mapping) {
+    can_find_gid = posix_group && !use_id_mapping;
+    if (can_find_gid) {
         ret = sysdb_attrs_get_uint32_t(group, map[SDAP_AT_GROUP_GID].sys_name,
                                        &gid);
     }
-    if (!posix_group || ret == ENOENT || (ret == EOK && gid == 0)) {
+    if (!can_find_gid || ret == ENOENT || (ret == EOK && gid == 0)) {
         DEBUG(SSSDBG_TRACE_ALL,
              "The group's gid was %s\n", ret == ENOENT ? "missing" : "zero");
         DEBUG(SSSDBG_TRACE_INTERNAL,
              "Marking group as non-posix and setting GID=0!\n");
+
         if (ret == ENOENT || !posix_group) {
             ret = sysdb_attrs_add_uint32(group,
                                          map[SDAP_AT_GROUP_GID].sys_name, 0);
-- 
2.1.0



More information about the sssd-devel mailing list