[SSSD] [PATCHES] support local overrides

Lukas Slebodnik lslebodn at redhat.com
Mon Jul 27 10:26:03 UTC 2015


On (27/07/15 12:13), Jakub Hrozek wrote:
>On Mon, Jul 27, 2015 at 12:04:33PM +0200, Pavel Březina wrote:
>> On 07/26/2015 09:40 PM, Jakub Hrozek wrote:
>> >On Sun, Jul 26, 2015 at 08:14:22PM +0200, Jakub Hrozek wrote:
>> >>On Fri, Jul 24, 2015 at 01:08:17PM +0200, Pavel Březina wrote:
>> >>>https://fedorahosted.org/sssd/ticket/2584
>> >>>
>> >>>If you have any idea how to improve manual page, please, share it.
>> >>
>> >>Hi,
>> >>
>> >>please see my comments inline.
>> >
>> >btw the patches work more or less fine -- great work, thanks.
>> >
>> >The only functional error is the missing check for NULL res value if an
>> >entry is found by getpwnam() but not sysdb_getpwnam():
>> >
>> >[root at client ~]# sss_override user-add root -n toor
>> >ldb: unable to dlopen /usr/lib64/ldb/modules/ldb/memberof.la :
>> >/usr/lib64/ldb/modules/ldb/memberof.la: invalid ELF header
>> >Segmentation fault (core dumped)
>> 
>> I think it was problem of accessing dom rather than res. Check is there.
>
>Thanks.
>
>> 
>> >After testing, I put the patches through Coverity. Here are the errors
>> >(some were already mentioned in the review):
>> 
>> I fixed all of them, hopefully, except:
>> 
>> >Error: DEADCODE (CWE-561): [#def6]
>> >sssd-1.13.1/src/tools/sss_override.c:302: equality_cond: Jumping to case "SYSDB_MEMBER_GROUP".
>> >sssd-1.13.1/src/tools/sss_override.c:331: equality_cond: Jumping to case "SYSDB_MEMBER_GROUP".
>> >sssd-1.13.1/src/tools/sss_override.c:296: equality_cond: Jumping to case "SYSDB_MEMBER_USER".
>> >sssd-1.13.1/src/tools/sss_override.c:325: equality_cond: Jumping to case "SYSDB_MEMBER_USER".
>> >sssd-1.13.1/src/tools/sss_override.c:324: between: When switching on "type", the value of "type" must be between 0 and 1.
>> >sssd-1.13.1/src/tools/sss_override.c:324: dead_error_condition: The switch value "type" cannot reach the default case.
>> >sssd-1.13.1/src/tools/sss_override.c:337: dead_error_begin: Execution cannot reach this statement: "default:".
>> >#  335|               strtype = "group";
>> >#  336|               break;
>> >#  337|->         default:
>> >#  338|               DEBUG(SSSDBG_CRIT_FAILURE, "Unsupported member type %d\n", type);
>> >#  339|               ret = ERR_INTERNAL;
>> >
>> 
>> Yes, it is a dead code. But I think it's better to leave it in:
>> 1) Without default a compiler will produce warning since not all
>> SYSDB_MEMBER* are used.
>> 2) Better to have it.
>> 
>> Do you think it may be better to use all SYSDB_MEMBER* instead of default
>> branch? It will however also produce the coverity warning, I think.
>
>I think this is OK and it's better to be paranoid and fail.
>
>Just be prepared there will be a Coverity warning next Monday on the
>internal list :-) and when it arrives, mark the Coverity error as a
>false positive..
>
It's not a false positive. "false positive" means bug in static analysers.
It should be marked as an intentional.

LS



More information about the sssd-devel mailing list