[SSSD] [PATCH] ldap_child: fix coverity warning

Pavel Reichl preichl at redhat.com
Tue Mar 10 14:59:17 UTC 2015



On 03/10/2015 10:40 AM, Lukas Slebodnik wrote:
> On (09/03/15 11:24), Pavel Reichl wrote:
>> Hello,
>>
>> please see attached trivial patch fixing 2 coverity warnings - ret was
>> overriden without being read first.
>>
>>> done:
>>>     if (krberr != 0) KRB5_SYSLOG(krberr);
>>>     if (keytab) krb5_kt_close(context, keytab);
>>>     if (context) krb5_free_context(context);
>>>     if (ccname_file_dummy) {
>>>     DEBUG(SSSDBG_TRACE_INTERNAL, "Unlinking [%s]\n", ccname_file_dummy);
>>>         ret = unlink(ccname_file_dummy);
>>>         if (ret == -1) {
>>>             ret = errno;
>>>             DEBUG(SSSDBG_MINOR_FAILURE,
>>>                   "Unlink failed [%d][%s].\n", ret, strerror(ret));
>>>         }
>>>     }
>>>     talloc_free(tmp_ctx);
>>>     return krberr;
>> ci:
>>
>> http://sssd-ci.duckdns.org/logs/job/9/03/summary.html
>>
>>
>> Thanks!
> >From f2faea03742598aa8185db6deeaa0b23b4fa1e52 Mon Sep 17 00:00:00 2001
>> From: Pavel Reichl <preichl at redhat.com>
>> Date: Mon, 9 Mar 2015 05:45:20 -0400
>> Subject: [PATCH] ldap_child: fix coverity warning
>>
>> In ldap_child_get_tgt_sync() variable 'ret' got overriden in done
>> section without ever before being read.
>> ---
>> src/providers/ldap/ldap_child.c | 6 ++++++
>> 1 file changed, 6 insertions(+)
>>
>> diff --git a/src/providers/ldap/ldap_child.c b/src/providers/ldap/ldap_child.c
>> index 8f034affa48095b6e512c866f8a3c33465e5c595..e9e845f35df53a780f365f90332a04338ad16026 100644
>> --- a/src/providers/ldap/ldap_child.c
>> +++ b/src/providers/ldap/ldap_child.c
>> @@ -399,6 +399,9 @@ static krb5_error_code ldap_child_get_tgt_sync(TALLOC_CTX *memctx,
>>                                          DB_PATH, realm_name);
>>      if (ccname_file_dummy == NULL) {
>>          ret = ENOMEM;
>> +        DEBUG(SSSDBG_CRIT_FAILURE,
>> +              "talloc_asprintf failed: %s:[%d].\n",
>> +              strerror(ret), ret);
>>          goto done;
>>      }
>>
>> @@ -407,6 +410,9 @@ static krb5_error_code ldap_child_get_tgt_sync(TALLOC_CTX *memctx,
>>      umask(old_umask);
>>      if (fd == -1) {
>>          ret = errno;
>> +        DEBUG(SSSDBG_CRIT_FAILURE,
>> +              "mkstemp failed: %s:[%d].\n",
>> +              strerror(ret), ret);
>>          goto done;
> ACK for debug messages.
> NACK for patch.
>
> As you can see ldap_child_get_tgt_sync returns krb5_error_code and not
> int or errnot_t.
>
> It means that function ldap_child_get_tgt_sync might return wrong error code.
> It's very likely that krberr was set to 0 in previous steps.
>
> LS
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Thanks, please see updated patch.

Would it make sense to file a ticket to refactor (break into smaller 
functions) ldap_child_get_tgt_sync()? I think it is too long and thus 
difficult to maintain.

ci:

http://sssd-ci.duckdns.org/logs/job/9/30/summary.html

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-ldap_child-fix-coverity-warning.patch
Type: text/x-patch
Size: 1913 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150310/3d80d368/attachment.bin>


More information about the sssd-devel mailing list