Bug 103737 - Make use of SO_PEERGROUPS in order to populate list of auxiliary groups
Summary: Make use of SO_PEERGROUPS in order to populate list of auxiliary groups
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: git master
Hardware: Other All
: medium normal
Assignee: Simon McVittie
QA Contact: D-Bus Maintainers
URL:
Whiteboard: review+
Keywords: patch
: 9328 97821 (view as bug list)
Depends on:
Blocks: 104657
  Show dependency treegraph
 
Reported: 2017-11-14 15:40 UTC by Michal Sekletar
Modified: 2018-05-09 15:18 UTC (History)
4 users (show)

See Also:
i915 platform:
i915 features:


Attachments
[1/11] DBusTransport, DBusConnection: Add internal getter for the credentials (4.02 KB, patch)
2018-01-16 13:47 UTC, Simon McVittie
Details | Splinter Review
sysdeps: Get complete group vector from Linux SO_PEERGROUPS if possible (14.77 KB, patch)
2018-01-16 13:48 UTC, Simon McVittie
Details | Splinter Review
[3/11] credentials: Add test coverage for groups (6.95 KB, patch)
2018-01-16 13:49 UTC, Simon McVittie
Details | Splinter Review
[4/11] _dbus_credentials_to_string_append: Remove useless join = FALSE (1.25 KB, patch)
2018-01-16 13:49 UTC, Simon McVittie
Details | Splinter Review
[5/11] credentials: Add test coverage for stringification (2.92 KB, patch)
2018-01-16 13:50 UTC, Simon McVittie
Details | Splinter Review
[6/11] DBusCredentials: Add _dbus_clear_credentials() (1.17 KB, patch)
2018-01-16 13:51 UTC, Simon McVittie
Details | Splinter Review
[7/11] loopback test: Display credentials received (1.64 KB, patch)
2018-01-16 13:53 UTC, Simon McVittie
Details | Splinter Review
[8/11] bus: Try to get groups directly from credentials, not userdb (1.65 KB, patch)
2018-01-16 13:54 UTC, Simon McVittie
Details | Splinter Review
[9/11] bus: Get loginfo string bits from DBusCredentials (4.09 KB, patch)
2018-01-16 13:56 UTC, Simon McVittie
Details | Splinter Review
[10/11] sysdeps: Document what _dbus_credentials_new_from_current_process has (1.88 KB, patch)
2018-01-16 13:58 UTC, Simon McVittie
Details | Splinter Review
[11/11] bus driver: Use DBusCredentials to fill credentials structure (8.83 KB, patch)
2018-01-16 13:59 UTC, Simon McVittie
Details | Splinter Review
[2/11 v2] sysdeps: Get complete group vector from Linux SO_PEERGROUPS if possible (14.78 KB, patch)
2018-01-16 14:58 UTC, Simon McVittie
Details | Splinter Review
[02/11 v3] sysdeps: Get complete group vector from Linux SO_PEERGROUPS if possible (15.50 KB, patch)
2018-02-16 17:02 UTC, Simon McVittie
Details | Splinter Review
[03/11 v2] credentials: Add test coverage for groups (7.24 KB, patch)
2018-02-16 17:02 UTC, Simon McVittie
Details | Splinter Review
[07/11 v2] loopback test: Display credentials received (2.02 KB, patch)
2018-02-16 17:03 UTC, Simon McVittie
Details | Splinter Review
[08/11 v2] bus: Try to get groups directly from credentials, not userdb (1.84 KB, patch)
2018-02-16 17:11 UTC, Simon McVittie
Details | Splinter Review
Fix null pointer dereferences in bus_connection_get_unix_groups (935 bytes, patch)
2018-03-20 08:54 UTC, Ralf Habacker
Details | Splinter Review
bus_connection_get_unix_groups: NULL-check *groups, not groups (908 bytes, patch)
2018-03-20 12:16 UTC, Ralf Habacker
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Michal Sekletar 2017-11-14 15:40:47 UTC
Recent linux kernels (linux-v4.13 and newer) allow for getting a list of auxiliary groups of a socket peer in a race-free way using SO_PEERGROUPS getsockopt option. For more details on SO_PEERGROUPS see the commit message,

https://patchwork.ozlabs.org/patch/778713/

dbus-daemon currently uses getgrouplist() when it is available. We should use SO_PEERGROUPS if possible and fallback to getgrouplist() otherwise.
Comment 1 Simon McVittie 2017-11-14 19:24:09 UTC
I'd be happy to review a patch, but I'm not intending to implement this myself any time soon (other priorities).

High-level design:

This would have to work a lot like add_linux_security_label_to_credentials(); then the groups would need to be propagated through the library in the same way that's currently done for the LSM label, so that bus_connection_get_unix_groups() could use that instead of looking up the uid in the DBusUserDatabase (a cache around the system passwd/group database, e.g. glibc nsswitch).

DBusCredentials would need to distinguish between:

- we do know the group vector (new Linux; group vector is returned
  directly, without consulting DBusUserDatabase)

- we don't know the group vector (non-Linux, old Linux; current
  implementation of bus_connection_get_unix_groups() is used)
Comment 2 Simon McVittie 2017-11-14 19:27:50 UTC
Does SO_PEERGROUPS guarantee to include the primary gid, or guarantee to include only the auxiliary/supplementary groups, or does it not make an API guarantee either way? (This is the same ambiguity as getgroups().)

If it doesn't, then the primary gid would have to come from the struct ucred, but only get put in the DBusCredentials if SO_PEERGROUPS worked (to avoid breaking the DBusUserDatabase case).
Comment 3 David Herrmann 2017-11-20 15:33:49 UTC
SO_PEERGROUPS returns the auxiliary groups of the current process as tracked by the kernel. On linux, this does *not* necessarily include the user's primary group. In most cases, you want to add it in after fetching the array.

Also note that the aux-groups cannot contain duplicates, and are unsorted.
Comment 4 Simon McVittie 2017-11-22 12:05:46 UTC
(In reply to David Herrmann from comment #3)
> SO_PEERGROUPS returns the auxiliary groups of the current process as tracked
> by the kernel. On linux, this does *not* necessarily include the user's
> primary group. In most cases, you want to add it in after fetching the array.

Thanks, that's useful information. So the primary group ID from the struct ucred would probably need to be passed in to add_groups_to_credentials() as a parameter so that it could be included in the DBusCredentials.
Comment 5 Simon McVittie 2018-01-16 13:47:33 UTC
Created attachment 136756 [details] [review]
[1/11] DBusTransport, DBusConnection: Add internal getter for  the credentials

We have a lot of dbus_connection_get_foo() and
_dbus_transport_get_foo() that are actually rather redundant.
Comment 6 Simon McVittie 2018-01-16 13:48:39 UTC
Created attachment 136757 [details] [review]
sysdeps: Get complete group vector from Linux SO_PEERGROUPS if possible
Comment 7 Simon McVittie 2018-01-16 13:49:34 UTC
Created attachment 136758 [details] [review]
[3/11] credentials: Add test coverage for groups
Comment 8 Simon McVittie 2018-01-16 13:49:57 UTC
Created attachment 136759 [details] [review]
[4/11] _dbus_credentials_to_string_append: Remove useless join = FALSE

It can't actually matter in practice, because we never know the
Unix uid and Unix groups but not pid, and we never have a Windows SID
and also a Linux security label; but resetting join to FALSE can only
ever result in us outputting something like "foo=123bar=456" instead
of the intended form with a space in the middle.
Comment 9 Simon McVittie 2018-01-16 13:50:35 UTC
Created attachment 136760 [details] [review]
[5/11] credentials: Add test coverage for stringification
Comment 10 Simon McVittie 2018-01-16 13:51:40 UTC
Created attachment 136762 [details] [review]
[6/11] DBusCredentials: Add _dbus_clear_credentials()

Not to be confused with _dbus_credentials_clear(), which does something
different (this is a little unfortunate, but the fact that they take
different types should clarify which is which).

---

This is analogous to GLib g_clear_object(), and libdbus dbus_clear_message(), dbus_clear_connection() etc.: it unrefs (if not NULL) and nullifies a variable or struct member given as a pointer.
Comment 11 Simon McVittie 2018-01-16 13:53:37 UTC
Created attachment 136763 [details] [review]
[7/11] loopback test: Display credentials received

---

On a development laptop with Linux 4.14, strace says this is working as intended:

getsockopt(7, SOL_SOCKET, SO_PEERCRED, {pid=20344, uid=1000, gid=1000}, [12]) = 0
getsockopt(7, SOL_SOCKET, SO_PEERSEC, 0x55654423bc80, [1024]) = -1 ENOPROTOOPT (Protocol not available)
getsockopt(7, SOL_SOCKET, SO_PEERGROUPS, "\4\0\0\0\24..."..., [1024->72]) = 0

...

MSG: Credentials: uid=1000 pid=20344 gid=4 gid=20 ... gid=1000
Comment 12 Simon McVittie 2018-01-16 13:54:13 UTC
Created attachment 136764 [details] [review]
[8/11] bus: Try to get groups directly from credentials, not userdb

If we avoid consulting the userdb, then it's one less chance to
deadlock.
Comment 13 Simon McVittie 2018-01-16 13:56:15 UTC
Created attachment 136766 [details] [review]
[9/11] bus: Get loginfo string bits from DBusCredentials

This saves a couple of _dbus_strdup/dbus_free pairs.

---

I'm not using _dbus_credentials_to_string_append() here because that's a larger change, and would result in the loginfo containing (potentially many) group IDs and being rather long; but I could do that as a later change.

We'd still need to append comm (the command corresponding to the pid, as read by rummaging in /proc) if desired.
Comment 14 Simon McVittie 2018-01-16 13:58:08 UTC
Created attachment 136767 [details] [review]
[10/11] sysdeps: Document what  _dbus_credentials_new_from_current_process has

It only has the most important credentials, not the full set.

---

I think it's correct that we don't get the LSM label or the complete group vector here. We can't easily get the LSM label, and we rarely actually care about either of those.

The most important thing (which perhaps I should edit this commit to document) is that it does have everything that _dbus_credentials_same_user() needs.
Comment 15 Simon McVittie 2018-01-16 13:59:49 UTC
Created attachment 136768 [details] [review]
[11/11] bus driver: Use DBusCredentials to fill credentials  structure

---

I haven't tested it, but I think this gives us a small functional improvement: previously, GetConnectionCredentials("org.freedesktop.DBus") on Windows would not give us the dbus-daemon's Windows SID (I didn't implement that because I wasn't going to test it); but now we get that information "for free".
Comment 16 Simon McVittie 2018-01-16 14:19:48 UTC
*** Bug 97821 has been marked as a duplicate of this bug. ***
Comment 17 Simon McVittie 2018-01-16 14:28:21 UTC
Comment on attachment 136757 [details] [review]
sysdeps: Get complete group vector from Linux SO_PEERGROUPS if possible

Review of attachment 136757 [details] [review]:
-----------------------------------------------------------------

::: dbus/dbus-credentials.c
@@ +181,5 @@
> +static int
> +cmp_gidp (const void *a_, const void *b_)
> +{
> +  const gid_t *a = a_;
> +  const gid_t *b = b_;

This needs to be dbus_gid_t (they are not the same, dbus_gid_t is unsigned long and gid_t is whatever the kernel feels like using).

This broke the build for Windows, and would result in incorrect comparisons on 64-bit big-endian Unix machines (where unsigned long is typically 64-bit and gid_t is typically 32-bit).
Comment 18 Simon McVittie 2018-01-16 14:58:12 UTC
Created attachment 136770 [details] [review]
[2/11 v2] sysdeps: Get complete group vector from Linux SO_PEERGROUPS if possible

---

s/gid_t/dbus_gid_t/
Comment 19 Philip Withnall 2018-01-24 18:04:28 UTC
Comment on attachment 136756 [details] [review]
[1/11] DBusTransport, DBusConnection: Add internal getter for  the credentials

Review of attachment 136756 [details] [review]:
-----------------------------------------------------------------

r+
Comment 20 Philip Withnall 2018-01-24 18:09:40 UTC
Comment on attachment 136758 [details] [review]
[3/11] credentials: Add test coverage for groups

Review of attachment 136758 [details] [review]:
-----------------------------------------------------------------

Some minor comments/nitpicks.

::: dbus/dbus-credentials-util.c
@@ +46,5 @@
>  {
>    DBusCredentials *credentials;
> +  static const struct
> +    {
> +      int n;

unsigned?

@@ +80,5 @@
> +  if (group_vector)
> +    {
> +      dbus_gid_t *copy;
> +
> +      copy = dbus_new0 (dbus_gid_t, group_vectors[group_vector - 1].n);

Nitpick: Maybe add an `assert (group_vector > 0 && group_vector <= G_N_ELEMENTS (group_vectors))` at the top of the function? (Using a non-GLib equivalent for G_N_ELEMENTS() if you wish.)
Comment 21 Philip Withnall 2018-01-24 18:11:36 UTC
Comment on attachment 136759 [details] [review]
[4/11] _dbus_credentials_to_string_append: Remove useless join = FALSE

Review of attachment 136759 [details] [review]:
-----------------------------------------------------------------

r+ definitely
Comment 22 Philip Withnall 2018-01-24 18:13:56 UTC
Comment on attachment 136760 [details] [review]
[5/11] credentials: Add test coverage for stringification

Review of attachment 136760 [details] [review]:
-----------------------------------------------------------------

r+
Comment 23 Philip Withnall 2018-01-24 18:14:18 UTC
Comment on attachment 136762 [details] [review]
[6/11] DBusCredentials: Add _dbus_clear_credentials()

Review of attachment 136762 [details] [review]:
-----------------------------------------------------------------

r+, trivially
Comment 24 Philip Withnall 2018-01-24 18:16:05 UTC
Comment on attachment 136763 [details] [review]
[7/11] loopback test: Display credentials received

Review of attachment 136763 [details] [review]:
-----------------------------------------------------------------

r+ with one nitpick (feel free to ignore or push directly with a fix)

::: test/loopback.c
@@ +261,4 @@
>        test_main_context_iterate (f->ctx, TRUE);
>      }
>  
> +  creds = _dbus_connection_get_credentials (f->server_conn);

Nitpick: This block is ripe for a comment at the top summarising what it does. For example:

/* Log and superficially check the credentials at the server. */
Comment 25 Philip Withnall 2018-01-24 18:21:48 UTC
Comment on attachment 136764 [details] [review]
[8/11] bus: Try to get groups directly from credentials, not userdb

Review of attachment 136764 [details] [review]:
-----------------------------------------------------------------

A couple of comments.

::: bus/connection.c
@@ +1041,5 @@
>  
> +  credentials = _dbus_connection_get_credentials (connection);
> +
> +  if (credentials != NULL &&
> +      _dbus_credentials_get_unix_gids (credentials, &groups_borrowed,

Is it definitely always safe to convert between (const dbus_gid_t **) and (const unsigned long **) as you’re doing with groups_borrowed here?

@@ +1044,5 @@
> +  if (credentials != NULL &&
> +      _dbus_credentials_get_unix_gids (credentials, &groups_borrowed,
> +                                       n_groups))
> +    {
> +      int i;

unsigned? Though I guess it needs to match the type of `n_groups`, so ignore me.
Comment 26 Philip Withnall 2018-01-24 18:23:41 UTC
Comment on attachment 136766 [details] [review]
[9/11] bus: Get loginfo string bits from DBusCredentials

Review of attachment 136766 [details] [review]:
-----------------------------------------------------------------

r+
Comment 27 Philip Withnall 2018-01-24 18:24:52 UTC
Comment on attachment 136767 [details] [review]
[10/11] sysdeps: Document what  _dbus_credentials_new_from_current_process has

Review of attachment 136767 [details] [review]:
-----------------------------------------------------------------

r+
Comment 28 Philip Withnall 2018-01-24 18:32:46 UTC
Comment on attachment 136768 [details] [review]
[11/11] bus driver: Use DBusCredentials to fill credentials  structure

Review of attachment 136768 [details] [review]:
-----------------------------------------------------------------

r+
Comment 29 Philip Withnall 2018-01-24 18:51:59 UTC
Comment on attachment 136770 [details] [review]
[2/11 v2] sysdeps: Get complete group vector from Linux SO_PEERGROUPS if possible

Review of attachment 136770 [details] [review]:
-----------------------------------------------------------------

r- with a couple of questions.

::: dbus/dbus-credentials.c
@@ +49,5 @@
>  struct DBusCredentials {
>    int refcount;
>    dbus_uid_t unix_uid;
> +  dbus_gid_t *unix_gids;
> +  int n_unix_gids;

unsigned? Although I imagine by this point in the patch series, you’ll be quite bored of repeating to me whatever justification you have for leaving it signed.

::: dbus/dbus-sysdeps-unix.c
@@ +1824,5 @@
> +    }
> +
> +  /* Allocate an extra space for the primary group ID */
> +  n_gids = ((size_t) len) / sizeof (gid_t);
> +  converted_gids = dbus_new (dbus_gid_t, n_gids + 1);

Might it be possible for (n_gids + 1) to overflow if (len == MAXINT * sizeof (gid_t))?

(I realise this is a corner case of corner cases.)

@@ +1826,5 @@
> +  /* Allocate an extra space for the primary group ID */
> +  n_gids = ((size_t) len) / sizeof (gid_t);
> +  converted_gids = dbus_new (dbus_gid_t, n_gids + 1);
> +
> +  if (converted_gids == 0)

s/0/NULL/?
Comment 30 Simon McVittie 2018-01-24 23:06:36 UTC
(In reply to Philip Withnall from comment #20)
> ::: dbus/dbus-credentials-util.c
> @@ +46,5 @@
> >  {
> >    DBusCredentials *credentials;
> > +  static const struct
> > +    {
> > +      int n;
> 
> unsigned?

I suppose so; libdbus uses signed int for array lengths and indices, but this isn't libdbus API/ABI.

> Nitpick: Maybe add an `assert (group_vector > 0 && group_vector <=
> G_N_ELEMENTS (group_vectors))` at the top of the function? (Using a non-GLib
> equivalent for G_N_ELEMENTS() if you wish.)

Yeah, good idea. This is libdbus code, so no GLib, but we already have an "I can't believe it's not G_N_ELEMENTS" macro.

(In reply to Philip Withnall from comment #25)
> Is it definitely always safe to convert between (const dbus_gid_t **) and
> (const unsigned long **) as you’re doing with groups_borrowed here?

Yes, dbus_gid_t is hard-coded to be unsigned long (and I deliberately didn't add a cast, so the compiler would stop us if that changed). But it would be more obviously correct if I changed the type, and perhaps added a static assertion that assigning a dbus_gid_t to an unsigned long is lossless.

> unsigned? Though I guess it needs to match the type of `n_groups`, so ignore
> me.

Yes, in this case it needs to match the type of n_groups, and libdbus conventionally uses signed int. I believe there might be some sort of, possibly spurious, rationale about making sure that decrementing past 0 doesn't result in libdbus thinking a massive amount of memory has been allocated and being happy to overflow a buffer to an arbitrary extent? If we do that then we're already living in undefined-behaviour-land, which is why I think that rationale might be spurious; but it's too late to make the ints in our public API be size_t, and the internal interfaces might as well be consistent with the public API.

"""
The GString feature of accepting negative numbers for "length of string" is also absent, because it could keep us from detecting bogus huge lengths. i.e. if we passed in some bogus huge length it would be taken to mean "current length of string" instead of "broken crack"
""" — DBusString API documentation

(In reply to Philip Withnall from comment #29)
> > +  int n_unix_gids;
> 
> unsigned?

Same as previous.

> ::: dbus/dbus-sysdeps-unix.c
> @@ +1824,5 @@
> > +    }
> > +
> > +  /* Allocate an extra space for the primary group ID */
> > +  n_gids = ((size_t) len) / sizeof (gid_t);
> > +  converted_gids = dbus_new (dbus_gid_t, n_gids + 1);
> 
> Might it be possible for (n_gids + 1) to overflow if (len == MAXINT * sizeof
> (gid_t))?

If the Linux kernel gives us multiple gigabytes of group IDs then I think we have more significant problems than this, not least of which is that Linux's socklen_t is int, so len can't get larger than MAXINT however hard we try. (There's a Linus rant somewhere about how POSIX briefly tried to change the type that is now socklen_t from the int traditionally used in BSD sockets to size_t, and how that would have been an unacceptable ABI break; so I think it's highly unlikely that Linux socklen_t on a new architecture will be larger than int.)

I could add a static assertion, or make the function fail early if len gets comically huge, if you want. 65536 supplementary groups ought to be enough for anyone, and indeed that's as many as current Linux will ever let you have (even when 31-bit group IDs are enabled).

> s/0/NULL/?

Yes, good catch.
Comment 31 Simon McVittie 2018-02-16 17:02:12 UTC
Created attachment 137397 [details] [review]
[02/11 v3] sysdeps: Get complete group vector from Linux SO_PEERGROUPS if possible

---

Use size_t for number of group IDs.

Use NULL rather than 0 when checking whether malloc failed.

Early-return if the kernel claims we have some ridiculous number of
group IDs, to guarantee we don't overflow.
Comment 32 Simon McVittie 2018-02-16 17:02:53 UTC
Created attachment 137398 [details] [review]
[03/11 v2] credentials: Add test coverage for groups

---

Assert that group_vector is in range. Use size_t for number of groups.
Comment 33 Simon McVittie 2018-02-16 17:03:28 UTC
Created attachment 137399 [details] [review]
[07/11 v2] loopback test: Display credentials received


Reviewed-by: Philip Withnall <withnall@endlessm.com>
[smcv: Add a comment as requested]
Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 34 Simon McVittie 2018-02-16 17:11:17 UTC
Created attachment 137400 [details] [review]
[08/11 v2] bus: Try to get groups directly from credentials, not  userdb

---

Be more careful with types
Comment 35 Simon McVittie 2018-02-16 19:38:32 UTC
I have also tested these on a live system and they seem to work.
Comment 36 Philip Withnall 2018-02-21 12:08:14 UTC
Comment on attachment 137397 [details] [review]
[02/11 v3] sysdeps: Get complete group vector from Linux SO_PEERGROUPS if possible

Review of attachment 137397 [details] [review]:
-----------------------------------------------------------------

r+
Comment 37 Philip Withnall 2018-02-21 12:09:22 UTC
Comment on attachment 137398 [details] [review]
[03/11 v2] credentials: Add test coverage for groups

Review of attachment 137398 [details] [review]:
-----------------------------------------------------------------

r+
Comment 38 Philip Withnall 2018-02-21 12:10:03 UTC
Comment on attachment 137399 [details] [review]
[07/11 v2] loopback test: Display credentials received

Review of attachment 137399 [details] [review]:
-----------------------------------------------------------------

r+
Comment 39 Philip Withnall 2018-02-21 12:11:02 UTC
Comment on attachment 137400 [details] [review]
[08/11 v2] bus: Try to get groups directly from credentials, not  userdb

Review of attachment 137400 [details] [review]:
-----------------------------------------------------------------

r+
Comment 40 Simon McVittie 2018-03-02 15:18:41 UTC
Fixed in git for 1.13.4.
Comment 41 Ralf Habacker 2018-03-20 07:40:29 UTC
Comment on attachment 137400 [details] [review]
[08/11 v2] bus: Try to get groups directly from credentials, not  userdb

Review of attachment 137400 [details] [review]:
-----------------------------------------------------------------

::: bus/connection.c
@@ +1056,5 @@
> +
> +      *n_groups = n;
> +      *groups = dbus_new (unsigned long, n);
> +
> +      if (groups == NULL)

coverity mentioned that this is an issue at CID 265358:

*groups is referenced in line 1058 but afterwards groups is checked against null. If the intention is to check the return value of dbus_new it should be 

if (*groups == NULL)

otherwise the check need to be placed before line 1058
Comment 42 Ralf Habacker 2018-03-20 08:54:56 UTC
Created attachment 138213 [details] [review]
Fix null pointer dereferences in bus_connection_get_unix_groups

Null-checking "groups" suggests that it may be null, but it has already
been dereferenced on all paths leading to the check.

Coverity ID 265358.

Bug:
Comment 43 Simon McVittie 2018-03-20 11:13:45 UTC
Comment on attachment 138213 [details] [review]
Fix null pointer dereferences in bus_connection_get_unix_groups

Review of attachment 138213 [details] [review]:
-----------------------------------------------------------------

The change is obviously correct (thanks!), the commit message not so much.

> Fix null pointer dereferences in bus_connection_get_unix_groups
>
> Null-checking "groups" suggests that it may be null, but it has already
> been dereferenced on all paths leading to the check.

The interface of this function is that it takes groups != NULL, and all callers do that (at least I'm fairly sure they do), so dereferencing groups is never a NULL dereference. 

The null pointer dereference here is that on an OOM condition, we might dereference (*groups) while *that* pointer is NULL.

So I think a better commit message would be something like this:

"""
bus_connection_get_unix_groups: NULL-check *groups, not groups

groups is never NULL here, but *groups can be NULL on OOM, and that's the
check that was intended.
"""
Comment 44 Simon McVittie 2018-03-20 11:23:36 UTC
(In reply to Simon McVittie from comment #43)
> So I think a better commit message would be something like this:
> 
> """
> bus_connection_get_unix_groups: NULL-check *groups, not groups

or perhaps

bus_connection_get_unix_groups: Correct check for OOM
Comment 45 Ralf Habacker 2018-03-20 12:16:50 UTC
Created attachment 138221 [details] [review]
bus_connection_get_unix_groups: NULL-check *groups, not groups

groups is never NULL here, but *groups can be NULL on OOM, and that's the
check that was intended.

Coverity ID 265358.

- fixed commit message
Comment 46 Simon McVittie 2018-03-20 12:24:16 UTC
Comment on attachment 138221 [details] [review]
bus_connection_get_unix_groups: NULL-check *groups, not groups

Review of attachment 138221 [details] [review]:
-----------------------------------------------------------------

Ideal, please apply
Comment 47 Ralf Habacker 2018-03-20 12:32:01 UTC
- last patch applied to master
Comment 48 Simon McVittie 2018-05-09 15:18:22 UTC
*** Bug 9328 has been marked as a duplicate of this bug. ***


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.