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)
After testing, I put the patches through Coverity. Here are the errors
(some were already mentioned in the review):
Error: UNINIT (CWE-457): [#def1]
sssd-1.13.1/src/tools/common/sss_tools.c:249: var_decl: Declaring variable "pc"
without initializer.
sssd-1.13.1/src/tools/common/sss_tools.c:321: uninit_use_in_call: Using
uninitialized value "pc" when calling "poptFreeContext".
# 319|
# 320| done:
# 321|-> poptFreeContext(pc);
# 322| talloc_free(help);
# 323| return ret;
Error: COMPILER_WARNING: [#def2]
sssd-1.13.1/src/tools/common/sss_tools.c:321:5: warning: 'pc' may be used
uninitialized in this function [-Wmaybe-uninitialized]
# poptFreeContext(pc);
# ^
# 319|
# 320| done:
# 321|-> poptFreeContext(pc);
# 322| talloc_free(help);
# 323| return ret;
Error: UNINIT (CWE-457): [#def3]
sssd-1.13.1/src/tools/common/sss_tools.c:250: var_decl: Declaring variable
"ret" without initializer.
sssd-1.13.1/src/tools/common/sss_tools.c:323: uninit_use: Using uninitialized
value "ret".
# 321| poptFreeContext(pc);
# 322| talloc_free(help);
# 323|-> return ret;
# 324| }
# 325|
Error: COMPILER_WARNING: [#def4]
sssd-1.13.1/src/tools/common/sss_tools.c: scope_hint: In function
'sss_tool_popt_ex'
sssd-1.13.1/src/tools/common/sss_tools.c:323:5: warning: 'ret' may be used
uninitialized in this function [-Wmaybe-uninitialized]
# return ret;
# ^
# 321| poptFreeContext(pc);
# 322| talloc_free(help);
# 323|-> return ret;
# 324| }
# 325|
Error: UNINIT (CWE-457): [#def5]
sssd-1.13.1/src/tools/sss_override.c:203: var_decl: Declaring variable "ret"
without initializer.
sssd-1.13.1/src/tools/sss_override.c:253: uninit_use: Using uninitialized value
"ret".
# 251|
# 252| done:
# 253|-> if (ret != EOK) {
# 254| talloc_free(attrs);
# 255| return NULL;
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;
Error: FORWARD_NULL (CWE-476): [#def7]
sssd-1.13.1/src/tools/sss_override.c:363: var_compare_op: Comparing "dom" to
null implies that "dom" might be null.
sssd-1.13.1/src/tools/sss_override.c:365: var_deref_op: Dereferencing null
pointer "dom".
# 363| } while (check_next && dom != NULL);
# 364|
# 365|-> DEBUG(SSSDBG_TRACE_FUNC, "Domain of %s %s is %s\n",
# 366| strtype, name, dom->name);
# 367|
Error: UNINIT (CWE-457): [#def8]
sssd-1.13.1/src/tools/sss_override.c:458: var_decl: Declaring variable
"in_transaction" without initializer.
sssd-1.13.1/src/tools/sss_override.c:528: uninit_use: Using uninitialized value
"in_transaction".
# 526|
# 527| done:
# 528|-> if (in_transaction) {
# 529| sret = sysdb_transaction_cancel(domain->sysdb);
# 530| if (sret != EOK) {
Error: COMPILER_WARNING: [#def9]
sssd-1.13.1/src/tools/sss_override.c: scope_hint: In function
'override_object_del.isra.1'
sssd-1.13.1/src/tools/sss_override.c:528:8: warning: 'in_transaction' may be
used uninitialized in this function [-Wmaybe-uninitialized]
# if (in_transaction) {
# ^
# 526|
# 527| done:
# 528|-> if (in_transaction) {
# 529| sret = sysdb_transaction_cancel(domain->sysdb);
# 530| if (sret != EOK) {
Also you forgot to package the new tool and the new man page in the specfile.
And finally, some questions/proposals after testing:
- Do you think it makes sense to also add -find and -show commands?
- I wonder if we should explicitly barf if the user triying to override
UID to 0 ? I did test it and it's not possible, sssd returns the original
UID, which is good, I just wonder about a nicer error message.
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
