Bug 8952 - setgroups being passed a -1 group is causing crashes.
setgroups being passed a -1 group is causing crashes.
Status: RESOLVED FIXED
Product: Samba 3.6
Classification: Unclassified
Component: Winbind
3.6.5
All Other
: P5 regression
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
: 8646 (view as bug list)
Depends on:
Blocks: 8595
  Show dependency treegraph
 
Reported: 2012-05-21 21:28 UTC by Ira Cooper
Modified: 2012-05-26 20:05 UTC (History)
6 users (show)

See Also:


Attachments
patch for 3.6 (2.25 KB, patch)
2012-05-21 21:28 UTC, Ira Cooper
no flags Details
patch for master. (2.24 KB, patch)
2012-05-21 21:29 UTC, Ira Cooper
no flags Details
Fix for 3.6, take 2. (590 bytes, patch)
2012-05-22 12:47 UTC, Ira Cooper
no flags Details
Negative caching fix for 3.6. (598 bytes, patch)
2012-05-22 15:53 UTC, Ira Cooper
metze: review-
Details
Negative caching fix for 3.6, alternate. (615 bytes, patch)
2012-05-22 16:12 UTC, Ira Cooper
no flags Details
Fix for 3.6 (568 bytes, patch)
2012-05-22 17:22 UTC, Ira Cooper
no flags Details
Fix for master. (5.32 KB, patch)
2012-05-24 01:45 UTC, Ira Cooper
metze: review-
Details
Fix for 3.6. (979 bytes, patch)
2012-05-24 01:46 UTC, Ira Cooper
no flags Details
Patches for v3-6-test (4.11 KB, patch)
2012-05-24 08:36 UTC, Stefan Metzmacher
ira: review-
Details
Additional DEBUG Patches for v3-6-test (12.56 KB, patch)
2012-05-24 08:40 UTC, Stefan Metzmacher
ira: review-
Details
Patch for v3-6-test (1.12 KB, patch)
2012-05-24 12:05 UTC, Stefan Metzmacher
ira: review+
Details
Fix for master. (987 bytes, patch)
2012-05-24 12:34 UTC, Ira Cooper
metze: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ira Cooper 2012-05-21 21:28:53 UTC
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.)
Comment 1 Ira Cooper 2012-05-21 21:29:20 UTC
Created attachment 7576 [details]
patch for master.
Comment 2 Stefan Metzmacher 2012-05-22 05:02:51 UTC
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.
Comment 3 Ira Cooper 2012-05-22 12:47:43 UTC
Created attachment 7577 [details]
Fix for 3.6, take 2.
Comment 4 Stefan Metzmacher 2012-05-22 12:55:51 UTC
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
Comment 5 Ira Cooper 2012-05-22 15:53:46 UTC
Created attachment 7578 [details]
Negative caching fix for 3.6.
Comment 6 Stefan Metzmacher 2012-05-22 16:01:32 UTC
Comment on attachment 7578 [details]
Negative caching fix for 3.6.

We should fix the caching layer to return WBC_ID_TYPE_NOT_SPECIFIED
Comment 7 Ira Cooper 2012-05-22 16:12:38 UTC
Created attachment 7579 [details]
Negative caching fix for 3.6, alternate.
Comment 8 Ira Cooper 2012-05-22 16:15:31 UTC
(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.
Comment 9 Ira Cooper 2012-05-22 16:30:15 UTC
I'll -1 that one.  It started having issues on my systems.
Comment 10 Ira Cooper 2012-05-22 17:22:50 UTC
Created attachment 7580 [details]
Fix for 3.6
Comment 11 Ira Cooper 2012-05-22 17:26:13 UTC
metze: The 3.6 change should meet your desires here.
Comment 12 SATOH Fumiyasu 2012-05-23 01:24:57 UTC
I think this bug is a duplicate of Bug 8646
(invalid group (-1) using idmap backend nss panics sys_setgroups on solaris)
Comment 13 Ira Cooper 2012-05-24 01:45:48 UTC
Created attachment 7586 [details]
Fix for master.
Comment 14 Ira Cooper 2012-05-24 01:46:29 UTC
Created attachment 7587 [details]
Fix for 3.6.
Comment 15 Ira Cooper 2012-05-24 01:48:44 UTC
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 16 Stefan Metzmacher 2012-05-24 05:47:45 UTC
Comment on attachment 7586 [details]
Fix for master.

wrong patch
Comment 17 Stefan Metzmacher 2012-05-24 08:36:45 UTC
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?
Comment 18 Stefan Metzmacher 2012-05-24 08:40:08 UTC
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 19 Ira Cooper 2012-05-24 10:33:12 UTC
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 20 Ira Cooper 2012-05-24 10:34:52 UTC
Comment on attachment 7591 [details]
Additional DEBUG Patches for v3-6-test

Expired and negatie cache entries are common.  Do not clutter my logs.
Comment 21 Ira Cooper 2012-05-24 10:39:44 UTC
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.
Comment 22 Stefan Metzmacher 2012-05-24 12:05:32 UTC
Created attachment 7592 [details]
Patch for v3-6-test

Then let's just use this one for 3.6, ok?
Comment 23 Ira Cooper 2012-05-24 12:34:39 UTC
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.
Comment 24 Stefan Metzmacher 2012-05-24 12:44:53 UTC
(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...
Comment 25 Stefan Metzmacher 2012-05-24 12:47:24 UTC
Ira, please add the review flag in the 3.6 patch and assign the
bug to Karolin <ks@sernet.de>
Comment 26 Stefan Metzmacher 2012-05-24 13:21:11 UTC
Comment on attachment 7593 [details]
Fix for master.

Fine for master (I have it in my private branch already)
Comment 27 Ira Cooper 2012-05-24 13:25:29 UTC
Comment on attachment 7592 [details]
Patch for v3-6-test

Passed testing in our setup.  Looks good.
Comment 28 Ira Cooper 2012-05-24 13:28:08 UTC
*** Bug 8646 has been marked as a duplicate of this bug. ***
Comment 29 Stefan Metzmacher 2012-05-26 08:58:43 UTC
(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
Comment 30 Karolin Seeger 2012-05-26 20:05:41 UTC
Pushed to v3-6-test.
Closing out bug report.

Thanks!