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@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 _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
