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:
- 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..
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?
In the future, yes. We may want them.
Please file a ticket. I suspect we'll get a request, so it might be nice to acknowledge the issue.
- 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.
Or a man page change? Or both? It is not in this patchset...
Both would be best, it's not like we're going to change that behaviour, ever, so once we write these two, they're to stay :-) It doesn't have to be part of this patchset either.