Created attachment 7575 [details] patch for 3.6 sys_getgroups is getting called with an array containing -1, thus causing crashes on our NexentaCore/Illumos machines. The root cause of this for us was groups having sIDHistory records, so when we went to look them up by objectSid, the lookup failed. This code allows us to lookup by sIDHistory or objectSid as needed. Patches against master and 3.6 enclosed for review. The 3.6 patch actually has been run, and looks good. The patch against master can not be run in my environment due to issues in master, that look totally unrelated to this patch. (And I'm not holding the patch, given that it stops crashes.)
Created attachment 7576 [details] patch for master.
Comment on attachment 7576 [details] patch for master. Doesn't that mean we get two sids mapped to the same uid/gid? sIDHistory can have multiple values. The debug messages should print the sid otherwise they're useless.
Created attachment 7577 [details] Fix for 3.6, take 2.
Comment on attachment 7577 [details] Fix for 3.6, take 2. 14:55 < metze> ira: the bug is that we get WBC_ID_TYPE_GID at all 14:55 < metze> if we can't map it it should be WBC_ID_TYPE_NOT_SPECIFIED 14:55 < metze> then the if statement before will catch it
Created attachment 7578 [details] Negative caching fix for 3.6.
Comment on attachment 7578 [details] Negative caching fix for 3.6. We should fix the caching layer to return WBC_ID_TYPE_NOT_SPECIFIED
Created attachment 7579 [details] Negative caching fix for 3.6, alternate.
(In reply to comment #6) > Comment on attachment 7578 [details] > Negative caching fix for 3.6. > > We should fix the caching layer to return WBC_ID_TYPE_NOT_SPECIFIED. Try the alternate.
I'll -1 that one. It started having issues on my systems.
Created attachment 7580 [details] Fix for 3.6
metze: The 3.6 change should meet your desires here.
I think this bug is a duplicate of Bug 8646 (invalid group (-1) using idmap backend nss panics sys_setgroups on solaris)
Created attachment 7586 [details] Fix for master.
Created attachment 7587 [details] Fix for 3.6.
Metze, these should be as we agreed on IRC. I'm concerned about the master, because I can't test it due to bugs in master. The 3.6 patch has been tested.
Comment on attachment 7586 [details] Fix for master. wrong patch
Created attachment 7590 [details] Patches for v3-6-test This might fixes also the winbindd internal usage of the idmap cache. Ira: I've fixed leading whitespaces and a compiler warning in your patch. Also, you should not upload the output of 'git show', please use 'git format-patch --stdout ...', as that will work with 'git am' then. Can you test this patches?
Created attachment 7591 [details] Additional DEBUG Patches for v3-6-test This adds some more DEBUG information to diagnose problems with broken idmap entries (this applies on top of attachment 7590 [details]).
Comment on attachment 7590 [details] Patches for v3-6-test a3bb188df65ba1cdd179bc8481f923db6ddf8ed0: This is a separate issue. Please track it alone. It is good to have found. But it is not the root cause of this error. -1. 7a64da3189a1e901aff66aa5e2e46e4a9c03750f: This is really a noise change. I want this over with. +1 3ae160287dfd8c089ea321c38ceea93e02b5552f: DEBUG should be 5 at least, more likely 10, I altered that in later patches. It was only at 1 briefly. This is a common case. You made a style change that hides the real change. Just put the debug statement right in the switch, this is a 1 line patch, and the length of time it took me to figure that out was too long. Don't make a style change in a change this simple, IMHO. Also, I'd contend this change is unneeded. -1.
Comment on attachment 7591 [details] Additional DEBUG Patches for v3-6-test Expired and negatie cache entries are common. Do not clutter my logs.
jra: I need mediation here. This has gone beyond a code review into pedantry/shedding, IMHO. I appreciate a good review. But this is beyond.
Created attachment 7592 [details] Patch for v3-6-test Then let's just use this one for 3.6, ok?
Created attachment 7593 [details] Fix for master. This patch mirrors your patch for 3.6. It still has the issue that I can't test it.
(In reply to comment #23) > Created attachment 7593 [details] > Fix for master. > > This patch mirrors your patch for 3.6. It still has the issue that I can't > test it. I'll push that to master...
Ira, please add the review flag in the 3.6 patch and assign the bug to Karolin <ks@sernet.de>
Comment on attachment 7593 [details] Fix for master. Fine for master (I have it in my private branch already)
Comment on attachment 7592 [details] Patch for v3-6-test Passed testing in our setup. Looks good.
*** Bug 8646 has been marked as a duplicate of this bug. ***
(In reply to comment #26) > Comment on attachment 7593 [details] > Fix for master. > > Fine for master (I have it in my private branch already) The fix is in master as 074991cefe2b8bb58de869e099379e182fab28b7
Pushed to v3-6-test. Closing out bug report. Thanks!