Thanks, it is an elegant solution to switch to growing the buffer by a multiplier if returned ngroups is unchanged.

I would retry in a loop with *= 2 instead of *= 16, which has the advantage of not allocating something far too large, and also of still working if someone is in a huge number of groups. And can get rid of this:

+              _dbus_warn("It appears that username \"%s\" is in more than %d groups.\nProceeding with just the first %d groups.",
+                         username_c, buf_count, buf_count);

Rather than warn, should just avoid the situation.

Created attachment 21638 [details] [review]
fallback on primary GID if using AD or LDAP

I ran into this problem on building GIMP and fixed it but just have forgotten to post the patch. Sorry.

I fixed it by simply using the primary GID if getgroulist() fails. There was no need to increase buf_count first. A patchfile is attached.

The problem only occured if a user acount is bound to Active Directory or LDAP. As far as I can see, there's no problem with more then 16 groups if AD or LDAP isn't uesd.

Again, sorry for not posting the patch in the first place.

Many dbus configurations may not really use group-based permissions, by falling back to primary gid you are just disabling group-based permissions, which may be OK on a particular system depending on what you use, but it isn't a real solution to the bug. If primary gid were good enough we would not be bothering with getgrouplist() in the first place ;-)

(In reply to comment #1)
> Thanks, it is an elegant solution to switch to growing the buffer by a
> multiplier if returned ngroups is unchanged.
> 
> I would retry in a loop with *= 2 instead of *= 16, which has the advantage of
> not allocating something far too large, and also of still working if someone is
> in a huge number of groups. And can get rid of this:
> 
> +              _dbus_warn("It appears that username \"%s\" is in more than %d
> groups.\nProceeding with just the first %d groups.",
> +                         username_c, buf_count, buf_count);
> 
> Rather than warn, should just avoid the situation.
> 

Thanks for your attention to this.. in the current patch I tried to stay with the current structure of a single retry, since I'm new to this code.  Are you requesting that in order to get this considered for inclusion in a future release, I should supply an alternate patch which instead loops to grow the buffer by successive doublings until the call succeeds?  I'd be happy to do so, if you would please just give me guidance as to your preferred set of termination conditions for that loop: obviously success is one, but if the getgrouplist() call continues to return a negative value, should we just continue doubling until we exhaust memory?  That does not seem to me like polite behavior for a library.  If there is going to be a fixed small limit to the number of iterations (such as 4-10 doublings, say) then we might as well go right to the max on the second try as the current patch does, since the code just after this patched portion copies the group information into another structure of the exact size needed and frees the buffer passed to getgrouplist(). Allocating and then releasing several KB does not seem like much of a resource waste these days.  I guess if we have a fixed moderate limit (like 20 doublings) it's worth looping, to avoid unnecessarily allocating several megabytes and releasing it.  If you agree that there should be another termination condition besides success or out of memory, then if you could let me know whether the library should abort or warn in such a case, that would be very helpful.  If you feel it is OK for the code to assume that the getgrouplist will eventually return >=0 with a large enough buffer, and that it's OK to loop until memory is exhausted if that assumption fails, then these last points are moot, but please confirm that would be your desired design for this small section of code.

Thanks for your further guidance, and I look forward to getting a fix for this into a future release of dbus.  Yours, Glen

I'm just suggesting improvements to the patch; if you have time to do them that's awesome and much appreciated, otherwise someone else may come along and do them when they have time or it's bugging them also.

To spell out my thinking a bit more, according to all docs I can find for getgrouplist(), the only reason it returns -1 is too-small buffer. So, I think we should assume the only bug we know of is that on OS X it does not return the buffer count, and that all platforms correctly return >= 0 if the buffer was large enough. (At least, until proven otherwise.) The docs I can find do not say that getgrouplist() ever does anything with errno.

The implication is, I don't think we need another termination condition. We should assume getgrouplist() works properly (other than the one bug we're working around here).

The alternative is basically insanity - I mean, we have to assume the C library works, we can't try to predict what random bugs it might have.

If we do learn about another known bug in a specific platform, we can work around it once we know what it is, just as we are for this bug.

Is this still needed now we have launchd included?

(In reply to comment #6)
> Is this still needed now we have launchd included?

No idea; in the long intervening time I have switched jobs (to heading up MoMath, the coutnry's only museum of mathematics) and have not had time for gnucash or dbus internals in quite a while.  So I have lost total track of this issue. Sorry.

dbus-launch should not be used on Mac OS X. It is intended to be called from login scripts to launch a session bus for the current session. This is exactly what launchd's "LaunchAgents" are for on OS-X. Dbus-1.4.1 finally supports this bus-launching method, although patches for earlier version were widely used since more then two years, see #14259.

However, the question of only using the primary GID or not is neither launchd, nor OS-X specific.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/dbus/dbus/issues/12.