Bug 54445 - Add generic GetConnectionCredentials
Summary: Add generic GetConnectionCredentials
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: 1.5
Hardware: All All
: medium enhancement
Assignee: D-Bus Maintainers
QA Contact: D-Bus Maintainers
URL:
Whiteboard: needs Windows test fixes
Keywords:
Depends on: 68496 68506
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-03 15:12 UTC by Simon McVittie
Modified: 2015-02-11 15:29 UTC (History)
7 users (show)

See Also:
i915 platform:
i915 features:


Attachments
1/3] bus driver: factor out common code to get a named connection (7.59 KB, patch)
2012-09-03 15:14 UTC, Simon McVittie
Details | Splinter Review
2/3] Convert a{sv} helpers from Stats into generic utility code (24.33 KB, patch)
2012-09-03 15:14 UTC, Simon McVittie
Details | Splinter Review
3/3] GetConnectionCredentials: add (10.88 KB, patch)
2012-09-03 15:15 UTC, Simon McVittie
Details | Splinter Review
a sketch of how to add Solaris ADT, SELinux and Windows credentials (5.11 KB, patch)
2012-09-03 15:18 UTC, Simon McVittie
Details | Splinter Review
a sketch of how to add Solaris ADT, SELinux and Windows credentials - with windows implementation (6.00 KB, patch)
2012-09-03 18:54 UTC, Ralf Habacker
Details | Splinter Review
a sketch of how to add Solaris ADT, SELinux and Windows credentials - fixed windows implementation (5.65 KB, patch)
2012-09-03 20:03 UTC, Ralf Habacker
Details | Splinter Review
windows credential implementation (749 bytes, patch)
2012-09-04 19:30 UTC, Ralf Habacker
Details | Splinter Review
windows credential implementation - free's string (841 bytes, patch)
2012-09-04 20:13 UTC, Ralf Habacker
Details | Splinter Review
Windows implemention of GetConnectionCredentials (Reviewed) (2.08 KB, patch)
2012-09-06 19:05 UTC, Ralf Habacker
Details | Splinter Review
[1] Convert a{sv} helpers from Stats into generic utility code (23.69 KB, patch)
2013-02-27 19:42 UTC, Simon McVittie
Details | Splinter Review
[2] GetConnectionCredentials: add (11.85 KB, patch)
2013-02-27 19:43 UTC, Simon McVittie
Details | Splinter Review
Windows implementation of GetConnectionCredentials. (2.06 KB, patch)
2013-02-27 19:44 UTC, Simon McVittie
Details | Splinter Review
[3] Document GetAdtAuditSessionData and GetConnectionSELinuxSecurityContext (5.29 KB, patch)
2013-02-27 19:44 UTC, Simon McVittie
Details | Splinter Review
[WiP] sketch of how to implement Solaris ADT, SELinux context (4.22 KB, patch)
2013-02-27 19:46 UTC, Simon McVittie
Details | Splinter Review
Windows implementation of GetConnectionCredentials (with utf8 validation check) (2.16 KB, patch)
2013-02-28 12:40 UTC, Ralf Habacker
Details | Splinter Review
Retrieve pid and sid from tcp connection peer (7.12 KB, patch)
2013-03-06 14:56 UTC, Ralf Habacker
Details | Splinter Review
Windows implementation of GetConnectionCredentials (fixed) (2.38 KB, patch)
2013-03-06 14:59 UTC, Ralf Habacker
Details | Splinter Review
[1] Convert a{sv} helpers from Stats into generic utility code (23.69 KB, patch)
2013-06-24 13:34 UTC, Simon McVittie
Details | Splinter Review
[2] GetConnectionCredentials: add (11.85 KB, patch)
2013-06-24 13:35 UTC, Simon McVittie
Details | Splinter Review
[3] Document GetAdtAuditSessionData and GetConnectionSELinuxSecurityContext (5.29 KB, patch)
2013-06-24 13:35 UTC, Simon McVittie
Details | Splinter Review
windows implementation of GetConnectionCredential (update) (2.55 KB, patch)
2013-06-25 07:26 UTC, Ralf Habacker
Details | Splinter Review
[PATCH] Fix build with "--enable-stats" (1.47 KB, patch)
2013-07-18 07:55 UTC, Chengwei Yang
Details | Splinter Review
[PATCH v2] Fix build with "--enable-stats" (1013 bytes, patch)
2013-07-19 02:06 UTC, Chengwei Yang
Details | Splinter Review
Add testcase for windows sid to test dbus-daemon (2.87 KB, patch)
2013-08-24 13:07 UTC, Ralf Habacker
Details | Splinter Review
Test for windows sid (update) (2.87 KB, patch)
2013-08-28 17:34 UTC, Ralf Habacker
Details | Splinter Review
Assert when not seen WindowsSID on windows (604 bytes, patch)
2013-08-28 17:38 UTC, Ralf Habacker
Details | Splinter Review
Export _dbus_getsid for test applications (1.25 KB, patch)
2013-08-28 17:57 UTC, Ralf Habacker
Details | Splinter Review
Test for windows sid (rebased) (2.45 KB, patch)
2015-02-10 15:25 UTC, Ralf Habacker
Details | Splinter Review
Minor optimization in _dbus_getsid() (1.40 KB, patch)
2015-02-10 15:27 UTC, Ralf Habacker
Details | Splinter Review
Minor optimization in _dbus_getsid() (update) (1.52 KB, patch)
2015-02-10 17:57 UTC, Ralf Habacker
Details | Splinter Review
Add test for Windows SID (update) (2.48 KB, patch)
2015-02-10 22:51 UTC, Ralf Habacker
Details | Splinter Review
bus_driver_handle_get_connection_credentials: do not assert on OOM (974 bytes, patch)
2015-02-11 14:24 UTC, Simon McVittie
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Simon McVittie 2012-09-03 15:12:37 UTC
So far we have separate methods to get:

* the uid on Unix systems (documented)
* the pid on Unix systems (documented)
* the SID on Windows systems (undocumented in the spec, but it returns
  a string SID, e.g. "S-1-5-18" for LOCAL_SYSTEM)
* the SELinux context (an undocumented method returning a blob of bytes
  in an undocumented format)
* the Solaris (?) audit session data (an undocumented method returning
  a blob of bytes in an undocumented format)

On Bug #47581, Brian wants to add yet another method, this time for Smack, which I believe is Linux-specific. I don't really want to add it at all, and certainly not on the o.fd.DBus interface.

The Maemo libcreds developers had a patch to add yet another method, returning their libcreds identifiers (I forget what format that was in), again Linux-specific. Again, I resisted adding it to the o.fd.DBus interface.

There are a number of problems with one-method-per-credential:

* Platforms that don't have a particular form of credentials end up
  with methods that are completely useless to them. For instance,
  Ralf has complained that the SELinux stuff is pretty useless on
  Windows (Bug #32407).

* If you're something like PolicyKit and you want to find out all the
  credentials of a process, you have to call up to 5 methods
  (assuming Linux with an unknown LSM), which seems rather silly.
  D-Bus normally encourages round-trip reduction.

* The majority of the credentials-getting methods are undocumented.
  Perhaps if you're a SELinux expert, there is an obvious, intuitive
  interpretation of a byte-blob as a SELinux context, but I'm not a
  SELinux expert.

* The majority of the code in the credentials-getting methods is
  uninteresting boilerplate.

I propose to add this method:

    GetConnectionCredentials (s: Bus_Name) -> a{sv}: Credentials

which returns all of the credentials in one go. Each LSM (or whatever) can define its own payload for this method.

In-tree functionality should use a short name like "UnixUserID" and document it in the D-Bus Specification.

Out-of-tree patches can either document their extensions in the D-Bus Specification, or use a reversed-domain prefix like org.maemo.Libcreds.
Comment 1 Simon McVittie 2012-09-03 15:14:26 UTC
Created attachment 66552 [details] [review]
1/3] bus driver: factor out common code to get a named  connection
Comment 2 Simon McVittie 2012-09-03 15:14:46 UTC
Created attachment 66554 [details] [review]
2/3] Convert a{sv} helpers from Stats into generic utility  code
Comment 3 Simon McVittie 2012-09-03 15:15:01 UTC
Created attachment 66555 [details] [review]
3/3] GetConnectionCredentials: add

The initial set of credentials is just UnixUserID and UnixProcessID.
The rest can follow when someone is sufficiently interested to actually
test them.
Comment 4 Simon McVittie 2012-09-03 15:16:42 UTC
Comment on attachment 66555 [details] [review]
3/3] GetConnectionCredentials: add

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

::: doc/dbus-specification.xml
@@ +5637,5 @@
> +            <tbody>
> +              <row>
> +                <entry>UnixUserID</entry>
> +                <entry>UINT32</entry>
> +                <entry>The numeric Unix user ID, as defined by POSIX</entry>

Open issue for bearded Unix gurus: should this be an INT64 or UINT64?

@@ +5642,5 @@
> +              </row>
> +              <row>
> +                <entry>UnixProcessID</entry>
> +                <entry>UINT32</entry>
> +                <entry>The numeric Unix process ID, as defined by POSIX</entry>

Likewise.
Comment 5 Simon McVittie 2012-09-03 15:18:54 UTC
Created attachment 66559 [details] [review]
a sketch of how to add Solaris ADT, SELinux and Windows credentials

We should support and document the other forms of credentials currently supported with undocumented methods, but I run Linux without a LSM, so I can't test any of them. So, here's my untested code. Someone who cares about each of these platforms should finish them off.
Comment 6 Simon McVittie 2012-09-03 15:22:00 UTC
(In reply to comment #5)
> a sketch of how to add Solaris ADT, SELinux and Windows credentials

Ralf or other Windows people: I think the Windows parts of this patch are nearly implemented, if you want to finish them off and get that bit included. In particular, I documented what WindowsSID means.

Is the string form of a SID, e.g. "S-1-5-18", the most appropriate representation of a Windows user that we can have?
Comment 7 Ralf Habacker 2012-09-03 16:09:09 UTC
(In reply to comment #6)
> (In reply to comment #5)
> > a sketch of how to add Solaris ADT, SELinux and Windows credentials
> 
> Ralf or other Windows people: I think the Windows parts of this patch are
> nearly implemented, if you want to finish them off and get that bit included.
I will take a look

> In particular, I documented what WindowsSID means.
>
> Is the string form of a SID, e.g. "S-1-5-18", the most appropriate
> representation of a Windows user that we can have?

no, this is a so called "well known sid" - see http://msdn.microsoft.com/en-us/library/aa379649.aspx

the generic format is documented for example at http://en.wikipedia.org/wiki/Security_Identifier#Overview

The format of an SID can be illustrated using the following example: "S-1-5-21-3623811015-3361044348-30300820-1013";
S 	The string is a SID. 	
1 	The revision level (the version of the SID specification). 	
5 	The identifier authority value. 	
21-3623811015-3361044348-30300820 	Domain or local computer identifier 	
1013 	A Relative ID (RID).  (identifies user) 

I would change the doc to the following 

+                <entry>WindowsSID</entry>
+                <entry>STRING</entry>
+                <entry>The Windows security identifier in its string form,
+                  e.g. "S-1-5-21-3623811015-3361044348-30300820-1013" for 
+                  a domain or local computer user or "S-1-5-18" for the 
+                  LOCAL_SYSTEM user</entry>
Comment 8 Ralf Habacker 2012-09-03 17:49:55 UTC
Comment on attachment 66559 [details] [review]
a sketch of how to add Solaris ADT, SELinux and Windows credentials

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

I saw a glib based test case - dbus on windows is currently build without glib so there are only manual tests possible - how to do ?

::: bus/driver.c
@@ +1625,5 @@
> +#endif
> +
> +  /* FIXME: someone on a Windows system needs to implement and test this */
> +#if 0
> +  windows_sid = FIXME;

What should be filled in here ? 

In case you are refering to the current process user the following need to be filled in:

  char *windows_sid = NULL;
  dbus_getsid(&windows_sid);

@@ +1629,5 @@
> +  windows_sid = FIXME;
> +
> +  if (windows_sid != NULL)
> +    {
> +      if (!_dbus_asv_add_string (&array_iter, "WindowsSID", windows_sid))

{
        LocalFree(windows_sid);

@@ +1631,5 @@
> +  if (windows_sid != NULL)
> +    {
> +      if (!_dbus_asv_add_string (&array_iter, "WindowsSID", windows_sid))
> +        goto oom;
> +    }

}
LocalFree(windows_sid);
Comment 9 Ralf Habacker 2012-09-03 18:54:19 UTC
Created attachment 66569 [details] [review]
a sketch of how to add Solaris ADT, SELinux and Windows credentials - with windows implementation
Comment 10 Ralf Habacker 2012-09-03 20:03:31 UTC
Created attachment 66578 [details] [review]
a sketch of how to add Solaris ADT, SELinux and Windows credentials - fixed windows implementation

The previous patch contains an unrelated CMakelist.txt, which has been cleaned
Comment 11 Ralf Habacker 2012-09-03 20:27:28 UTC
(In reply to comment #10)
> Created attachment 66578 [details] [review] [review]
> a sketch of how to add Solaris ADT, SELinux and Windows credentials - fixed
> windows implementation
> 
> The previous patch contains an unrelated CMakelist.txt, which has been cleaned

Got a running version with this patches on windows (cross compiled on linux with mingw)
Running dbus-daemon and dbusviewer with wine and called GetConnectionCredentials "WindowsSID". 

qdbusviewer returned nothing and dbus-daemon reported  

11: [bus/driver.c(2079):bus_driver_handle_message] Driver got a method call: GetConnectionCredentials
11: [bus/driver.c(2102):bus_driver_handle_message] Found driver handler for GetConnectionCredentials
11: [bus/driver.c(62):bus_driver_get_conn_helper] asked for credentials of connection WindowsSID
11: [bus/driver.c(2128):bus_driver_handle_message] Driver handler returned failure
11: [dbus/dbus-connection.c(2934):dbus_connection_get_is_connected] LOCK
11: [dbus/dbus-connection.c(413):_dbus_connection_unlock] UNLOCK

I changed the related patch in bus/driver.c to 

//#ifdef DBUS_WIN
    {
      char *windows_sid = NULL;
      _dbus_getsid (&windows_sid);
      _dbus_verbose ("get windows sid %s\n", windows_sid);
      if (windows_sid != NULL)
        {
          if (!_dbus_asv_add_string (&array_iter, "WindowsSID", windows_sid))
            {
              LocalFree (windows_sid);
              goto oom;
            }
           LocalFree (windows_sid);
        }
    }
//#endif

to see if this is called anyway but did not found the term "get windows sid" in the dbus-daemon log file created with DBUS_VERBOSE=1
Comment 12 Simon McVittie 2012-09-04 10:53:11 UTC
Comment on attachment 66578 [details] [review]
a sketch of how to add Solaris ADT, SELinux and Windows credentials - fixed windows implementation

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

::: bus/driver.c
@@ +1536,5 @@
>    DBusMessageIter array_iter;
>    unsigned long ulong_val;
>    const char *service;
> +#if 0
> +  /* only used by unfinished bits - Solaris/SELinux */

I'd prefer it if you could do a patch that adds the Windows parts, and omits the unfinished Solaris/SELinux bits. People who like Solaris or SELinux can get a prototype from my earlier patch and make it work.

@@ +1623,5 @@
> +        goto oom;
> +    }
> +#endif
> +
> +#ifdef DBUS_WIN

In principle this shouldn't need to be #ifdef DBUS_WIN, because the "get the Windows SID of the thing at the other end" function should just return NULL on non-Windows.

@@ +1626,5 @@
> +
> +#ifdef DBUS_WIN
> +    {
> +      char *windows_sid = NULL;
> +      _dbus_getsid(&windows_sid);

This should get the SID of the process at the other end of the connection, not the SID of the bus daemon. If that's not something you can do on Windows then we should just leave it out, and you don't need to do anything.

::: doc/dbus-specification.xml
@@ +5650,5 @@
> +                <entry>STRING</entry>
> +                <entry>The Windows security identifier in its string form,
> +                e.g. "S-1-5-21-3623811015-3361044348-30300820-1013" for 
> +                domain or local computer user or "S-1-5-18" for the 
> +                LOCAL_SYSTEM user</entry>

Fair enough.

::: test/dbus-daemon.c
@@ +436,5 @@
> +          g_assert_cmpuint (dbus_message_iter_get_arg_type (&var_iter), ==,
> +              DBUS_TYPE_STRING);
> +          dbus_message_iter_get_basic (&var_iter, &sid);
> +          g_message ("%s of this process is %s", name, sid);
> +          /* FIXME: assert that it's correct */

The idea was that this should call _dbus_getsid() to see what the SID of this process is, and compare it with what the dbus-daemon says our SID is.
Comment 13 Simon McVittie 2012-09-04 10:57:27 UTC
(In reply to comment #7)
> > Is the string form of a SID, e.g. "S-1-5-18", the most appropriate
> > representation of a Windows user that we can have?
> 
> no, this is a so called "well known sid" - see
> http://msdn.microsoft.com/en-us/library/aa379649.aspx

Right, what I meant is: is "S-1-5-18" or "S-1-5-21-3623811015-3361044348-30300820-1013" a more appropriate format to represent a Windows user than "LOCAL_SYSTEM" or "Ralf"? And it sounds as though the answer is "yes".

(The corresponding thing on Unix is that we use a numeric uid like 0 or 1000 rather than the string name like root or smcv, because it's the number that's actually used when making authentication decisions.)
Comment 14 Simon McVittie 2012-09-04 10:59:25 UTC
(In reply to comment #11)
> Running dbus-daemon and dbusviewer with wine and called
> GetConnectionCredentials "WindowsSID". 

That should fail with NameHasNoOwner.

The right thing would be something like GetConnectionCredentials ":1.42" or GetConnectionCredentials "com.example.MyService". It should return a map from string to variant. With my three patches, that map will be empty on Windows; with your patch in addition, it should contain { "WindowsSID": "S-blahblahblah" }.
Comment 15 Ralf Habacker 2012-09-04 19:30:56 UTC
Created attachment 66626 [details] [review]
windows credential implementation

Used available function to retrieves the windows sid of a connection
Comment 16 Ralf Habacker 2012-09-04 19:33:05 UTC
(In reply to comment #13)
> (In reply to comment #7)
> > > Is the string form of a SID, e.g. "S-1-5-18", the most appropriate
> > > representation of a Windows user that we can have?
> > 
> > no, this is a so called "well known sid" - see
> > http://msdn.microsoft.com/en-us/library/aa379649.aspx
> 
> Right, what I meant is: is "S-1-5-18" or
> "S-1-5-21-3623811015-3361044348-30300820-1013" a more appropriate format to
> represent a Windows user than "LOCAL_SYSTEM" or "Ralf"? And it sounds as though
> the answer is "yes".

yes, names in the mentioned form are not unique. 

> (The corresponding thing on Unix is that we use a numeric uid like 0 or 1000
> rather than the string name like root or smcv, because it's the number that's
> actually used when making authentication decisions.)
yes
Comment 17 Ralf Habacker 2012-09-04 19:35:19 UTC
(In reply to comment #14)
> (In reply to comment #11)
> > Running dbus-daemon and dbusviewer with wine and called
> > GetConnectionCredentials "WindowsSID". 
> 
> That should fail with NameHasNoOwner.
> 
> The right thing would be something like GetConnectionCredentials ":1.42" or
> GetConnectionCredentials "com.example.MyService". It should return a map from
> string to variant. With my three patches, that map will be empty on Windows;
> with your patch in addition, it should contain { "WindowsSID": "S-blahblahblah"
> }.

Okay, I was misleaded by trying to call the function with org.freedesktop.DBus which returns 

13: [bus/connection.c(2269):bus_transaction_send_error_reply] Sending error reply org.freedesktop.DBus.Error.NameHasNoOwner "Could not get credentials of name 'org.freedesktop.DBus': no such name"
13: [bus/connection.c(2014):bus_transaction_send_from_driver] Sending (no interface) (no member) org.freedesktop.DBus.Error.NameHasNoOwner from driver
13: [bus/policy.c(1068):bus_client_policy_check_can_receive]   (policy) checking receive rules, eavesdropping = 0
Comment 18 Ralf Habacker 2012-09-04 20:13:40 UTC
Created attachment 66629 [details] [review]
windows credential implementation - free's string
Comment 19 Simon McVittie 2012-09-05 08:41:06 UTC
Comment on attachment 66629 [details] [review]
windows credential implementation - free's string

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

This can't be merged until someone reviews my first three patches on this bug and says "yes". Any opinion on those patches?

::: bus/driver.c
@@ +1569,5 @@
>      }
>  
> +  if (dbus_connection_get_windows_user (conn, &windows_sid))
> +    {
> +      if (!_dbus_asv_add_string (&array_iter, "WindowsSID", windows_sid))

Thanks, this looks better. It still needs a paragraph in the spec explaining what WindowsSID means - the one in your previous patch looked OK.

Is the SID guaranteed to be UTF-8? From the documentation on MSDN it looks as though it's always ASCII, which is a subset of UTF-8, so that's fine.

(If it's possible to get, say, Latin-1 characters in the SID, we'd have to send it as a byte array instead of a UTF-8 string.)
Comment 20 Thiago Macieira 2012-09-05 09:13:24 UTC
I'm going over them today (among other tasks).
Comment 21 Thiago Macieira 2012-09-05 10:00:08 UTC
Comment on attachment 66552 [details] [review]
1/3] bus driver: factor out common code to get a named  connection

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

Ship it
Comment 22 Thiago Macieira 2012-09-05 10:10:01 UTC
Comment on attachment 66554 [details] [review]
2/3] Convert a{sv} helpers from Stats into generic utility  code

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

::: bus/stats.c
@@ +1,3 @@
>  /* stats.c - statistics from the bus driver
>   *
> + * Copyright © 2011-2012 Nokia Corporation

Why are you adding a Nokia copyright here?

And I guess that mojibake is only Bugzilla, right?

@@ +59,4 @@
>    _dbus_list_get_stats (&in_use, &in_free_list, &allocated);
> +
> +  if (!_dbus_asv_add_uint32 (&arr_iter, "Serial", stats_serial++) ||
> +      !asv_add_uint32 (&arr_iter, "ListMemPoolUsedBytes", in_use) ||

You've removed asv_add_uint32. Looks like you forgot to add the "_dbus_" prefix to this line and the next two.
Comment 23 Thiago Macieira 2012-09-05 10:23:12 UTC
Comment on attachment 66555 [details] [review]
3/3] GetConnectionCredentials: add

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

It's fine otherwise. I'd even ship it.

::: doc/dbus-specification.xml
@@ +5637,5 @@
> +            <tbody>
> +              <row>
> +                <entry>UnixUserID</entry>
> +                <entry>UINT32</entry>
> +                <entry>The numeric Unix user ID, as defined by POSIX</entry>

I don't know any system using PIDs and UIDs larger than 32-bit. We could go for 64-bit just in case, if it's no harm (and I don't think it is).

Then again, we could also define that it could be either type and the caller should check.

PS: pid_t is technically signed because it's used in some functions like kill(2) and wait(2) where negative numbers have special meaning, usually process groups. But PIDs are always positive.
Comment 24 Thiago Macieira 2012-09-05 10:23:50 UTC
Comment on attachment 66554 [details] [review]
2/3] Convert a{sv} helpers from Stats into generic utility  code

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

::: dbus/dbus-asv-util.h
@@ +34,5 @@
> +                                          DBusMessageIter *arr_iter);
> +void         _dbus_asv_abandon           (DBusMessageIter *iter,
> +                                          DBusMessageIter *arr_iter);
> +
> +dbus_bool_t  _dbus_asv_open_entry        (DBusMessageIter *arr_iter,

You don't need to export all these functions. Some of them are internal and should be static.
Comment 25 Ralf Habacker 2012-09-06 19:05:06 UTC
Created attachment 66741 [details] [review]
Windows implemention of GetConnectionCredentials (Reviewed)
Comment 26 Ralf Habacker 2012-09-06 19:09:37 UTC
Comment on attachment 66555 [details] [review]
3/3] GetConnectionCredentials: add

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

> UnixProcessID.

does this covers the process id from a dbus connection on windows too - then I suggest to rename UnixProcessID to ProcessID otherwise to add a different WindowsProcessID
Comment 27 Simon McVittie 2012-09-07 09:19:10 UTC
(In reply to comment #26)
> does this covers the process id from a dbus connection on windows too

Not as far as I know. It's a Unix pid_t as returned by getpid() (a temporarily-unique non-negative integer per process). GetCurrentProcessId() on Windows seems to be analogous.

The thing that makes it useful on Unix, but probably not on Windows, is that by doing credentials-passing across a Unix socket, the dbus-daemon can find out who is at the other end of the connection (user ID, one group ID, and process ID) in a secure way - it isn't possible to fake your process ID in this message unless you are privileged (root or CAP_SYS_ADMIN).

That isn't possible over TCP, so I don't think the process ID would ever be available on Windows anyway? (If it is, we can always add a WindowsProcessID later.)

This credentials stuff is mostly of interest for consumption by security software like PolicyKit, on buses like the standard system bus that act as a security boundary - so to be honest it's not particularly relevant on Windows, where D-Bus isn't a standard OS component and there's no system bus.
Comment 28 Thiago Macieira 2012-09-07 12:44:12 UTC
(In reply to comment #27)
> Not as far as I know. It's a Unix pid_t as returned by getpid() (a
> temporarily-unique non-negative integer per process). GetCurrentProcessId() on
> Windows seems to be analogous.

Then we should rename and simply call it ProcessID.

> The thing that makes it useful on Unix, but probably not on Windows, is that by
> doing credentials-passing across a Unix socket, the dbus-daemon can find out
> who is at the other end of the connection (user ID, one group ID, and process
> ID) in a secure way - it isn't possible to fake your process ID in this message
> unless you are privileged (root or CAP_SYS_ADMIN).
> 
> That isn't possible over TCP, so I don't think the process ID would ever be
> available on Windows anyway? (If it is, we can always add a WindowsProcessID
> later.)

I don't see the problem of just doing it now. Whether the D-Bus server can get the remote connection's PID or not is completely irrelevant. If it manages to get it, it's a 32-bit unsigned integer anyway.
Comment 29 Simon McVittie 2012-11-09 12:09:40 UTC
(I'll try to get back to this sometime.)
Comment 30 Simon McVittie 2013-01-28 15:54:36 UTC
For future reference if someone other than me wants to finish my patches from this bug:

(In reply to comment #22)
> Why are you adding a Nokia copyright here?

I was maintaining D-Bus for Maemo/MeeGo Harmattan at the time I wrote it.

> And I guess that mojibake is only Bugzilla, right?

Yes, the actual patch is valid UTF-8.

(In reply to comment #28)
> Then we should rename and simply call it ProcessID.

That seems reasonable.

> I don't see the problem of just doing it now. Whether the D-Bus server can
> get the remote connection's PID or not is completely irrelevant. If it
> manages to get it, it's a 32-bit unsigned integer anyway.

Fair enough.
Comment 31 Simon McVittie 2013-02-27 19:39:28 UTC
Comment on attachment 66552 [details] [review]
1/3] bus driver: factor out common code to get a named  connection

pushed, thanks
Comment 32 Simon McVittie 2013-02-27 19:42:30 UTC
Created attachment 75650 [details] [review]
[1] Convert a{sv} helpers from Stats into generic utility  code

---

I believe this addresses all of Thiago's comments. _dbus_asv_open_entry, _dbus_asv_close_entry and _dbus_asv_abandon_entry are now static (but are still named the same, to reduce churn if/when we add credentials that have a more complex type).
Comment 33 Simon McVittie 2013-02-27 19:43:58 UTC
Created attachment 75651 [details] [review]
[2] GetConnectionCredentials: add

The initial set of credentials is just UnixUserID and ProcessID.
The rest can follow when someone is sufficiently interested to actually
test them.

---

ProcessID is still 32-bit unsigned, but now has non-Unix-specific naming in case it ever becomes implementable on Windows.
Comment 34 Simon McVittie 2013-02-27 19:44:34 UTC
Created attachment 75652 [details] [review]
Windows implementation of GetConnectionCredentials.

From: Ralf Habacker <ralf.habacker@freenet.de>
Reviewed-by: Simon McVittie <simon.mcvittie@collabora.co.uk>

---

Rebased.
Comment 35 Simon McVittie 2013-02-27 19:44:59 UTC
Created attachment 75653 [details] [review]
[3] Document GetAdtAuditSessionData and  GetConnectionSELinuxSecurityContext

These are only part of the DBus interface because dbus-daemon didn't
previously support multiple interfaces. I don't know enough about
either of these security frameworks to know what they return, but
perhaps one day someone who knows about Solaris or SELinux will tell
us...
Comment 36 Simon McVittie 2013-02-27 19:46:43 UTC
Created attachment 75654 [details] [review]
[WiP] sketch of how to implement Solaris ADT, SELinux context

Deliberately not a git-format-patch: I have not compiled this, let alone tested it. If anyone wants to implement either of these, this is a starting point.
Comment 37 Simon McVittie 2013-02-27 19:57:29 UTC
(In reply to comment #19)
> Is the SID guaranteed to be UTF-8? From the documentation on MSDN it looks
> as though it's always ASCII, which is a subset of UTF-8, so that's fine.

Ralf, could you confirm this please?

If you're not sure, I suppose we could always throw in a call to dbus_validate_utf8(), and just not include the SID in the reply (meaning "I don't know, if in doubt don't trust this process") if it turns out to be non-UTF-8, the same way I didn't include the Unix uid/pid if they're beyond 32-bit.

That'd probably be a good idea for other frameworks' identifiers too, actually.
Comment 38 Ralf Habacker 2013-02-27 20:26:30 UTC
(In reply to comment #37)
> (In reply to comment #19)
> > Is the SID guaranteed to be UTF-8? From the documentation on MSDN it looks
> > as though it's always ASCII, which is a subset of UTF-8, so that's fine.
> 
> Ralf, could you confirm this please?
> 
http://msdn.microsoft.com/en-us/library/windows/desktop/aa379602%28v=vs.85%29.aspx states two forms: 

1. standard string representation, which is definitive ASCII
2. string SID's listed on the mentioned page are also ASCII

> If you're not sure, I suppose we could always throw in a call to
> dbus_validate_utf8(), and just not include the SID in the reply (meaning "I
> don't know, if in doubt don't trust this process") if it turns out to be
> non-UTF-8, the same way I didn't include the Unix uid/pid if they're beyond
> 32-bit.

yes, seems to be the best way. 

> 
> That'd probably be a good idea for other frameworks' identifiers too,
> actually.

yes
Comment 39 Ralf Habacker 2013-02-28 12:40:13 UTC
Created attachment 75689 [details] [review]
Windows implementation of GetConnectionCredentials (with utf8 validation check)
Comment 40 Simon McVittie 2013-02-28 14:02:46 UTC
Comment on attachment 75689 [details] [review]
Windows implementation of GetConnectionCredentials (with utf8 validation check)

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

review+ (conditional on my patches, obviously).
Comment 41 Ralf Habacker 2013-02-28 21:18:17 UTC
(In reply to comment #40)
> Comment on attachment 75689 [details] [review] [review]
> Windows implementation of GetConnectionCredentials (with utf8 validation
> check)
> 
> Review of attachment 75689 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> review+

Just for the record, here is a test case to check the results. 
1. start daemon
    wine bin/dbus-daemon.exe --session --print-address  & 
   ----- output -----
    <connect-string-output> 

2. start test client 
    DBUS_SESSION_BUS_ADDRESS=<connect-string-output> wine bin/dbus-monitor.exe > out.log &

3. get connection name
    DBUS_SESSION_BUS_ADDRESS=<connect-string-output> wine bin/dbus-send.exe \
    --print-reply --reply-timeout=120000 --type=method_call \
    --dest='org.freedesktop.DBus' '/org/freedesktop/DBus' 
    org.freedesktop.DBus.ListNames
   ----- output -----
   method return sender=org.freedesktop.DBus -> dest=:1.35 reply_serial=2
   array [
      string "org.freedesktop.DBus"
      string ":1.35"
      string ":1.24"
   ]

3. call method
    DBUS_SESSION_BUS_ADDRESS=<connect-string-output> wine bin/dbus-send.exe \
    --print-reply --reply-timeout=120000 --type=method_call \
    --dest='org.freedesktop.DBus' '/org/freedesktop/DBus' \
    org.freedesktop.DBus.GetConnectionCredentials string::1.24
   ----- output -----
    method return sender=org.freedesktop.DBus -> dest=:1.36 reply_serial=2
    array [
      dict entry(
         string "ProcessID"
         variant             uint32 8
      )
      dict entry(
         string "WindowsSID"
         variant             string "S-1-5-21-0-0-0-1000"
      )
   ]
Comment 42 Ralf Habacker 2013-02-28 21:23:33 UTC
(In reply to comment #40)
> 
> (conditional on my patches, obviously).

hope Thiago will find the time to review :-)
Comment 43 Simon McVittie 2013-03-04 12:24:47 UTC
(In reply to comment #41)
>       dict entry(
>          string "ProcessID"
>          variant             uint32 8
>       )

Where did this pid come from?!

_dbus_read_credentials_socket() appears to be mis-implemented: it claims that every peer - even if it authenticated using the ANONYMOUS mechanism - has the pid and SID of the *current* process (the dbus-daemon, in this case).

This is wrong. I'll clone a bug.
Comment 44 Simon McVittie 2013-03-04 12:40:22 UTC
(In reply to comment #41)
> 2. start test client 
>     DBUS_SESSION_BUS_ADDRESS=<connect-string-output> wine
> bin/dbus-monitor.exe > out.log &

I assume the idea here is that this dbus-monitor...

>     org.freedesktop.DBus.GetConnectionCredentials string::1.24

... is the owner of :1.24.

>       dict entry(
>          string "ProcessID"
>          variant             uint32 8
>       )

GetConnectionCredentials is claiming that this is the pid of dbus-monitor. I don't think it actually is: I think it's the pid of dbus-daemon.

>       dict entry(
>          string "WindowsSID"
>          variant             string "S-1-5-21-0-0-0-1000"
>       )

This is your SID (user ID), which is presumably true for both the dbus-daemon and the dbus-monitor, unless someone else has joined your session bus.
Comment 45 Simon McVittie 2013-03-04 12:41:09 UTC
(In reply to comment #43)
> This is wrong. I'll clone a bug.

Bug #61787
Comment 46 Ralf Habacker 2013-03-04 12:56:15 UTC
(In reply to comment #43)
> (In reply to comment #41)
> >       dict entry(
> >          string "ProcessID"
> >          variant             uint32 8
> >       )
> 
> Where did this pid come from?!

Remember dbus-daemon has been running with wine. This is the windows pid for dbus-daemon.exe
> 
> _dbus_read_credentials_socket() appears to be mis-implemented: it claims
> that every peer - even if it authenticated using the ANONYMOUS mechanism -
> has the pid and SID of the *current* process (the dbus-daemon, in this case).
>
Comment 47 Simon McVittie 2013-03-04 14:55:33 UTC
(In reply to comment #46)
> Remember dbus-daemon has been running with wine. This is the windows pid for
> dbus-daemon.exe

Right. My complaint wasn't that this is a Wine-emulated Windows pid rather than a Linux pid - that's fine, and expected - but that providing the dbus-daemon pid here is not useful information, and not what GetConnectionCredentials is documented to do: GetConnectionCredentials(X) should return the credentials of the connected process X, not those of the dbus-daemon itself.

If that means that it can't return ProcessID on Windows, then it shouldn't return ProcessID on Windows - giving information that can't be trusted to be correct is worse than giving no information.

This wrong process ID is Bug #61787, which (IMO) shouldn't block this one: I believe GetConnectionUnixProcessID is equally wrong on Windows, for the same reason. If you can't tell the peer's process ID, which I don't think you can, then GCUPI should just raise an error.
Comment 48 Ralf Habacker 2013-03-06 14:56:54 UTC
Created attachment 76017 [details] [review]
Retrieve pid and sid from tcp connection  peer
Comment 49 Ralf Habacker 2013-03-06 14:59:04 UTC
Created attachment 76018 [details] [review]
Windows implementation of GetConnectionCredentials (fixed)
Comment 50 Simon McVittie 2013-03-06 15:33:41 UTC
Comment on attachment 76017 [details] [review]
Retrieve pid and sid from tcp connection  peer

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

This is actually a fix for Bug #61787 I think?

Some style points and a couple of simplifications, but I'm glad this is something we can do on Windows after all.

::: cmake/dbus/CMakeLists.txt
@@ +266,4 @@
>      if(WINCE)
>          target_link_libraries(dbus-1 ws2)
>      else(WINCE)
> +        target_link_libraries(dbus-1 ws2_32 advapi32 netapi32 iphlpapi)

If iphlpapi is necessary, please add -liphlpapi to NETWORK_libs in configure.ac too. (Search for ws2_32 to find the right place - there's only one instance.)

::: dbus/dbus-sysdeps-win.c
@@ +113,5 @@
> + */
> +int get_peer_pid_from_tcp_handle(int handle)
> +{
> +  struct sockaddr_storage addr;
> +  socklen_t len = sizeof addr;

Coding style: we usually say sizeof (thing) with the parentheses.

@@ +119,5 @@
> +  char peer_ipstr[INET6_ADDRSTRLEN];
> +
> +  DWORD result;
> +  DWORD size;
> +  MIB_TCPTABLE2 *tcpTable;

Coding style: I'd prefer tcp_table.

@@ +122,5 @@
> +  DWORD size;
> +  MIB_TCPTABLE2 *tcpTable;
> +  int i;
> +
> +  getpeername(handle, (struct sockaddr*)&addr, &len);

coding style: getpeername (handle, (struct sockaddr *) &addr...);

(and many more instances of the same things)

@@ +125,5 @@
> +
> +  getpeername(handle, (struct sockaddr*)&addr, &len);
> +
> +  /* deal with both IPv4 and IPv6: */
> +  if (addr.ss_family == AF_INET) {

indentation in D-Bus style, please

@@ +128,5 @@
> +  /* deal with both IPv4 and IPv6: */
> +  if (addr.ss_family == AF_INET) {
> +      struct sockaddr_in *s = (struct sockaddr_in *)&addr;
> +      peer_port = ntohs(s->sin_port);
> +      inet_ntop(AF_INET, &s->sin_addr, peer_ipstr, sizeof(peer_ipstr));

No need to use inet_ntop(), just compare s->sin_addr.s_addr with INADDR_LOOPBACK (both are in network byte order, iirc).

@@ +132,5 @@
> +      inet_ntop(AF_INET, &s->sin_addr, peer_ipstr, sizeof(peer_ipstr));
> +  } else { /* AF_INET6 */
> +      struct sockaddr_in6 *s = (struct sockaddr_in6 *)&addr;
> +      peer_port = ntohs(s->sin6_port);
> +      inet_ntop(AF_INET6, &s->sin6_addr, peer_ipstr, sizeof(peer_ipstr));

No need to use inet_ntop(), just memcmp it with in6addr_loopback. (Assuming Windows has that...)

@@ +136,5 @@
> +      inet_ntop(AF_INET6, &s->sin6_addr, peer_ipstr, sizeof(peer_ipstr));
> +  }
> +
> +  /* check for localhost */
> +  if (strcmp(peer_ipstr,"127.0.0.1") != 0)

This would be wrong for IPv6, but as noted, you don't need to use string comparisons anyway.

@@ +820,4 @@
>   * @returns process sid
>   */
>  static dbus_bool_t
> +_dbus_getsid(char **sid, int process_id)

space before (

Are Windows process IDs really "int", or should they be DWORD or something?

@@ +826,5 @@
>    TOKEN_USER *token_user = NULL;
>    DWORD n;
>    PSID psid;
>    int retval = FALSE;
> +  HANDLE process_handle = OpenProcess(PROCESS_ALL_ACCESS, FALSE, process_id);

Is this going to fail if we don't own the other process? Can we "open" it in a less enthusiastic mode, i.e. "just look at it"?

@@ +1761,5 @@
>    int bytes_read = 0;
>    DBusString buf;
> +  char *sid = NULL;
> +  int pid;
> +  int retval = TRUE;

I'd make this start FALSE, and set it to TRUE just before the "out" label.

@@ +1782,2 @@
>  
> +  _dbus_credentials_add_unix_pid(credentials, pid);

This should probably be renamed to ...add_pid if it's equally valid for Windows (but that can be a separate change).

@@ +1791,5 @@
> +  goto end;
> +
> +failed:
> +  retval = FALSE;
> +end:

The conventional label name for the end of a function in libdbus is "out" (I prefer "finally" personally, but libdbus already has a convention).

This can be simplified to this form, which I often use elsewhere - considerably less goto'ing around.

    dbus_bool_t retval = FALSE;
    ...
        if (bad thing)
          goto out;
    ...
        if (other bad thing)
          goto out;
    ...

    retval = TRUE;

  out:
    if (sid != NULL)
      LocalFree (sid);

    return retval;
  }
Comment 51 Ralf Habacker 2013-03-06 22:10:50 UTC
Comment on attachment 76017 [details] [review]
Retrieve pid and sid from tcp connection  peer

followups are assigned to bug 61787
Comment 52 Ralf Habacker 2013-04-29 18:49:40 UTC
(In reply to comment #51)
> Comment on attachment 76017 [details] [review] [review]
> Retrieve pid and sid from tcp connection  peer
> 
> followups are assigned to bug 61787

After this has been fixed, is there anything left to do for me ?
Comment 53 Simon McVittie 2013-06-20 12:40:15 UTC
(In reply to comment #52)
> After this has been fixed, is there anything left to do for me ?

You could review my three patches here? :-)
Comment 54 Simon McVittie 2013-06-20 12:44:13 UTC
Comment on attachment 76018 [details] [review]
Windows implementation of GetConnectionCredentials (fixed)

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

::: bus/driver.c
@@ +1572,5 @@
>      }
>  
> +  if (dbus_connection_get_windows_user (conn, &windows_sid))
> +    {
> +      if (_dbus_check_is_valid_utf8 (windows_sid))

Now that I look at this again: this is not actually correct. According to dbus-marshal-validate.h, _dbus_check_foo() is only meant to be used in _dbus_return_[val_]if_fail(), and in an attempt to enforce that, it isn't defined if checks are disabled.

(Virtually nobody actually sees that failure mode, because compiling libdbus without checks doesn't usually make sense.)

You could use dbus_validate_utf8 (windows_sid, NULL) from dbus-syntax.h, now that I've added it.
Comment 55 Ralf Habacker 2013-06-24 12:37:20 UTC
Comment on attachment 75650 [details] [review]
[1] Convert a{sv} helpers from Stats into generic utility  code

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

patch apply fails with recent dbus master source

apply: Convert a{sv} helpers from Stats into generic utility code
error: patch failed: bus/stats.c:193
error: bus/stats.c: patch does not apply
Patch failed at 0001 Convert a{sv} helpers from Stats into generic utility code
Comment 56 Simon McVittie 2013-06-24 13:28:41 UTC
(In reply to comment #55)
> patch apply fails with recent dbus master source

Yeah, it conflicts. I'll update it.
Comment 57 Simon McVittie 2013-06-24 13:34:46 UTC
Created attachment 81340 [details] [review]
[1] Convert a{sv} helpers from Stats into generic utility  code
Comment 58 Simon McVittie 2013-06-24 13:35:04 UTC
Created attachment 81341 [details] [review]
[2] GetConnectionCredentials: add
Comment 59 Simon McVittie 2013-06-24 13:35:21 UTC
Created attachment 81342 [details] [review]
[3] Document GetAdtAuditSessionData and  GetConnectionSELinuxSecurityContext
Comment 60 Ralf Habacker 2013-06-24 13:59:08 UTC
Comment on attachment 81340 [details] [review]
[1] Convert a{sv} helpers from Stats into generic utility  code

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

looks good
Comment 61 Ralf Habacker 2013-06-24 14:01:33 UTC
(In reply to comment #54)
> Comment on attachment 76018 [details] [review] [review]
> Windows implementation of GetConnectionCredentials (fixed)
> 
> Review of attachment 76018 [details] [review] [review]:
> -----------------------------------------------------------------

> You could use dbus_validate_utf8 (windows_sid, NULL) from dbus-syntax.h, now
> that I've added it.

There is the problem that dbus_validate_utf8 needs a DBusString and  windows_sid is a char pointer
Comment 62 Simon McVittie 2013-06-24 14:18:30 UTC
(In reply to comment #61)
> There is the problem that dbus_validate_utf8 needs a DBusString and 
> windows_sid is a char pointer

You can either use dbus_validate_utf8() (which takes a const char *), or _dbus_string_init_const() and _dbus_string_validate_utf8() (which takes a DBusString). _dbus_validate_utf8() is an alias for _dbus_string_validate_utf8(), but dbus_validate_utf8() (without the underscore) is a wrapper with a different signature.

I believe the justification for _dbus_check_is_valid_utf8() being unavailable when compiling without checks was that when parsing a binary D-Bus message, _dbus_string_validate_utf8() is the right thing, and _dbus_check_is_valid_utf8() would be wrong (because binary D-Bus messages aren't '\0'-terminated and it is necessary to check for out-of-bounds reading).

However, I added dbus_validate_utf8() (and similar functions) because it seemed stupid for bindings like dbus-python to have to reimplement them.

If you want to turn _dbus_check_is_valid_utf8 (x) into a macro wrapper around dbus_validate_utf8 (x, NULL), and so on, I wouldn't object.
Comment 63 Ralf Habacker 2013-06-25 07:26:18 UTC
Created attachment 81389 [details] [review]
windows implementation of GetConnectionCredential (update)
Comment 64 Simon McVittie 2013-06-25 11:20:51 UTC
Comment on attachment 81389 [details] [review]
windows implementation of GetConnectionCredential (update)

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

Looks good, if someone says "yes" to my patches on this bug.

::: bus/driver.c
@@ +1576,5 @@
> +      DBusString str;
> +      dbus_bool_t result;
> +      _dbus_string_init_const (&str, windows_sid);
> +      result = _dbus_validate_utf8 (&str, 0, _dbus_string_get_length (&str));
> +      _dbus_string_free (&str);

You don't actually need this _dbus_string_free() call, but it's OK either way.
Comment 65 Simon McVittie 2013-06-25 11:22:19 UTC
(In reply to comment #60)
> [1] Convert a{sv} helpers from Stats into generic utility  code
> looks good

Applied for 1.7.6, thanks
Comment 66 Ralf Habacker 2013-06-25 14:10:17 UTC
Comment on attachment 81341 [details] [review]
[2] GetConnectionCredentials: add

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

I found one function not very descriptive, which makes code reading harder.

Patch otherwise looks good. I would not say that the issue is a blocker.

::: test/dbus-daemon.c
@@ +311,4 @@
>  }
>  
>  static void
> +pc_store (DBusPendingCall *pc,

the name of the function is not very descriptive

@@ +428,5 @@
> +#endif
> +
> +#ifdef G_OS_WIN32
> +  /* FIXME: when implemented:
> +  g_assert (seen & SEEN_WINDOWS_SID);

the related windows patch need to address this
Comment 67 Ralf Habacker 2013-06-25 14:12:41 UTC
Comment on attachment 81342 [details] [review]
[3] Document GetAdtAuditSessionData and  GetConnectionSELinuxSecurityContext

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

formally this patch looks good.
Comment 68 Chengwei Yang 2013-07-18 07:55:50 UTC
Created attachment 82573 [details] [review]
[PATCH] Fix build with "--enable-stats"

Fix build failure introduced by these changes.
Comment 69 Simon McVittie 2013-07-18 09:25:23 UTC
Comment on attachment 82573 [details] [review]
[PATCH] Fix build with "--enable-stats"

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

::: bus/stats.c
@@ +166,3 @@
>        !_dbus_asv_add_uint32 (&arr_iter, "PeakBusNames",
>          bus_connection_get_peak_bus_names (stats_connection)) ||
>        !_dbus_asv_add_uint32 (&arr_iter, "UniqueName",

The bug here is that I should have been adding a string, not a uint32.

Adding the char * (pointer to the first byte in the string) to the packet as an integer makes no sense.

@@ +167,5 @@
>          bus_connection_get_peak_bus_names (stats_connection)) ||
>        !_dbus_asv_add_uint32 (&arr_iter, "UniqueName",
> +        /*
> +         * need a dbus_uint32_t but get a const char *, which sizeof
> +         * equals sizeof (unsigned long).

As a general note, this is not true in general: in particular, it's false on 64-bit Windows and on Linux "x32", which are LLP64 platforms.

(I don't personally think the performance gains for x32 are worth the latent bugs they will uncover, but some people want to use it anyway.)

@@ +173,5 @@
> +         * sizeof (unsigned long) != sizeof (unsigned int), so we'll
> +         * get error=pointer-to-int-cast
> +         *
> +         * However, we'll dead if we get an address larger than
> +         * UINT_MAX, Will it happen?

FYI: yes it can.
Comment 70 Chengwei Yang 2013-07-19 02:06:12 UTC
Created attachment 82643 [details] [review]
[PATCH v2] Fix build with "--enable-stats"

use _dbus_asv_add_string()
Comment 71 Simon McVittie 2013-08-22 17:18:54 UTC
Comment on attachment 82643 [details] [review]
[PATCH v2] Fix build with "--enable-stats"

Thanks, applied.
Comment 72 Simon McVittie 2013-08-23 10:22:16 UTC
(In reply to comment #64)
> windows implementation of GetConnectionCredential (update)

I think you accidentally included this in commit 600621dbc80. I reverted that commit, though, because the manual test indicated that it was incorrectly allowing ANONYMOUS too often.

I'd prefer to have the test enabled before merging this, if possible.

(In reply to comment #66)
> @@ +428,5 @@
> > +#endif
> > +
> > +#ifdef G_OS_WIN32
> > +  /* FIXME: when implemented:
> > +  g_assert (seen & SEEN_WINDOWS_SID);
> 
> the related windows patch need to address this

Please do - I don't have a development environment on Windows, so I can't really test it properly. The other thing that this test ought to check is that the SID we report is correct (basically, copy the logic for UnixUserID).
Comment 73 Ralf Habacker 2013-08-23 13:26:08 UTC
(In reply to comment #72)
> (In reply to comment #64)
> > windows implementation of GetConnectionCredential (update)
> 
> I think you accidentally included this in commit 600621dbc80. 
Happened while manual rebasing/patching the patches from #39720, seems to be caused by an unclear git environment in which an applied patch from this bug stays around - sorry for that. 

> (In reply to comment #66)
> > @@ +428,5 @@
> > > +#endif
> > > +
> > > +#ifdef G_OS_WIN32
> > > +  /* FIXME: when implemented:
> > > +  g_assert (seen & SEEN_WINDOWS_SID);
> > 
> > the related windows patch need to address this
> 
> Please do - I don't have a development environment on Windows, so I can't
> really test it properly. 

The problem here is, that dbus-glib, which is required, isn't available as a package on windows native nor as mingw32 cross compiled package yet. 

> The other thing that this test ought to check is that the SID we report is correct (basically, copy the logic for UnixUserID).

You are refering to  the compare to _DBUS_UINT32_MAX in 
+  if (dbus_connection_get_unix_user (conn, &ulong_val) &&
+      ulong_val <= _DBUS_UINT32_MAX)


the validation of the sid already happens in _dbus_getsid() from which all connection related sid's are fetch 
... 
  psid = token_user->User.Sid;
  if (!IsValidSid (psid))
    {
      _dbus_verbose("%s invalid sid\n",__FUNCTION__);
      goto failed;
    }
.
Comment 74 Simon McVittie 2013-08-23 13:34:53 UTC
(In reply to comment #73)
> The problem here is, that dbus-glib, which is required, isn't available as a
> package on windows native nor as mingw32 cross compiled package yet. 

It's reasonably easy to cross-compile, assuming you have a cross-compiled GLib to compile it against - that's what I do to smoke-test dbus/mingw.

> > The other thing that this test ought to check is that the SID we report is correct (basically, copy the logic for UnixUserID).
> 
> You are refering to  the compare to _DBUS_UINT32_MAX in 
> +  if (dbus_connection_get_unix_user (conn, &ulong_val) &&
> +      ulong_val <= _DBUS_UINT32_MAX)

No, I'm referring to the actual regression test in test/dbus-daemon.c:

      if (g_strcmp0 (name, "UnixUserID") == 0)
        {
#ifdef G_OS_UNIX
          guint32 u32;

          g_assert (!(seen & SEEN_UNIX_USER));
          g_assert_cmpuint (dbus_message_iter_get_arg_type (&var_iter), ==,
              DBUS_TYPE_UINT32);
          dbus_message_iter_get_basic (&var_iter, &u32);
          g_message ("%s of this process is %u", name, u32);
          g_assert_cmpuint (u32, ==, geteuid ());
          seen |= SEEN_UNIX_USER;
#else
          g_assert_not_reached ();
#endif
        }

There should be a similar block for WindowsSID which on Windows, checks that the WindowsSID is a string, gets the SID of the current process via Win32 API for comparison, checks that they are equal, and records SEEN_WINDOWS_SID; and on Unix, crashes with g_assert_not_reached().
Comment 75 Ralf Habacker 2013-08-23 13:51:42 UTC
(In reply to comment #74)
> (In reply to comment #73)
> > The problem here is, that dbus-glib, which is required, isn't available as a
> > package on windows native nor as mingw32 cross compiled package yet. 
> 
got it It has not been named dbus-glib instead it is named mingw32-dbus-1-glib

BTW: http://www.freedesktop.org/wiki/Software/DBusBindings/ states that dbus-glib is obsolate and gdbus should used. Does this apply to dbus too ?

> > > The other thing that this test ought to check is that the SID we report is correct (basically, copy the logic for UnixUserID).
> > 
> > You are refering to  the compare to _DBUS_UINT32_MAX in 
> > +  if (dbus_connection_get_unix_user (conn, &ulong_val) &&
> > +      ulong_val <= _DBUS_UINT32_MAX)
> 
> No, I'm referring to the actual regression test in test/dbus-daemon.c:
> 
>       if (g_strcmp0 (name, "UnixUserID") == 0)
>         {
> #ifdef G_OS_UNIX
>           guint32 u32;
> 
>           g_assert (!(seen & SEEN_UNIX_USER));
>           g_assert_cmpuint (dbus_message_iter_get_arg_type (&var_iter), ==,
>               DBUS_TYPE_UINT32);
>           dbus_message_iter_get_basic (&var_iter, &u32);
>           g_message ("%s of this process is %u", name, u32);
>           g_assert_cmpuint (u32, ==, geteuid ());
>           seen |= SEEN_UNIX_USER;
> #else
>           g_assert_not_reached ();
> #endif
>         }
> 
> There should be a similar block for WindowsSID which on Windows, checks that
> the WindowsSID is a string, gets the SID of the current process via Win32
> API for comparison, checks that they are equal, and records
> SEEN_WINDOWS_SID; and on Unix, crashes with g_assert_not_reached().
okay, that seem doable.
Comment 76 Ralf Habacker 2013-08-24 13:07:44 UTC
Created attachment 84562 [details] [review]
Add testcase for windows sid to test dbus-daemon

could not test yet because of blocker #68496
Comment 77 Simon McVittie 2013-08-27 11:09:05 UTC
Comment on attachment 84562 [details] [review]
Add testcase for windows sid to test dbus-daemon

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

::: test/dbus-daemon.c
@@ +81,5 @@
>    gchar *argv[] = {
>        binary,
>        configuration,
> +#ifdef DBUS_WIN
> +      "--print-address", /* stdout */

I'd prefer this:

     "--print-address=1", /* to stdout */
#ifdef DBUS_UNIX
     "--nofork",
#endif
     NULL
};

or if stdout isn't necessarily fd 1 in Windows, drop the "=1" everywhere to minimize the difference (bare --print-address is equivalent to --print-address=1 under Unix, so that's OK - I only included the "=1" to be more explicit).

@@ +409,5 @@
>  #endif
>          }
> +      else if (g_strcmp0 (name, "WindowsSID") == 0)
> +        {
> +#ifdef G_OS_WIN32

This is more or less what I had in mind, thanks.

@@ +421,5 @@
> +          dbus_message_iter_get_basic (&var_iter, &sid);
> +          g_message ("%s of this process is %s", name, sid);
> +          if (_dbus_getsid (&self_sid, 0))
> +              result = strcmp (self_sid, sid);
> +          g_assert_cmpuint (result, ==, 0);

I'd prefer:

     if (_dbus_getsid (&self_sid, 0))
         g_error ("_dbus_getsid() failed");

     g_assert_cmpstr (self_sid, ==, sid);

since that prints better diagnostics if they differ, or if one of them is NULL. (g_error() is always fatal, and g_assert_cmpstr() is a macro wrapper around g_strcmp0().)
Comment 78 Ralf Habacker 2013-08-28 17:31:02 UTC
(In reply to comment #77)
> Comment on attachment 84562 [details] [review] [review]
> Add testcase for windows sid to test dbus-daemon
> 
> Review of attachment 84562 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: test/dbus-daemon.c
> @@ +81,5 @@
> >    gchar *argv[] = {
> >        binary,
> >        configuration,
> > +#ifdef DBUS_WIN
> > +      "--print-address", /* stdout */
> 
> I'd prefer this:
> 
>      "--print-address=1", /* to stdout */
> #ifdef DBUS_UNIX
>      "--nofork",
> #endif
>      NULL
> };
> 
> or if stdout isn't necessarily fd 1 in Windows, drop the "=1" everywhere to
> minimize the difference (bare --print-address is equivalent to
> --print-address=1 under Unix, so that's OK - I only included the "=1" to be
> more explicit).

I remember that with =1 the daemon could not be started, need to recheck. 


> I'd prefer:
> 
>      if (_dbus_getsid (&self_sid, 0))

You probably meant 
       if  (!_dbus_getsid (&self_sid, 0))

>          g_error ("_dbus_getsid() failed");
> 
>      g_assert_cmpstr (self_sid, ==, sid);
> 
> since that prints better diagnostics if they differ, or if one of them is
> NULL. (g_error() is always fatal, and g_assert_cmpstr() is a macro wrapper
> around g_strcmp0().)
Comment 79 Ralf Habacker 2013-08-28 17:34:50 UTC
Created attachment 84801 [details] [review]
Test for windows sid (update)
Comment 80 Ralf Habacker 2013-08-28 17:38:12 UTC
Created attachment 84802 [details] [review]
Assert when not seen WindowsSID on windows

test complete
Comment 81 Ralf Habacker 2013-08-28 17:57:59 UTC
Created attachment 84804 [details] [review]
Export _dbus_getsid for test applications

required for patch #8481
Comment 82 Simon McVittie 2013-08-29 09:43:59 UTC
Comment on attachment 84804 [details] [review]
Export _dbus_getsid for test applications

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

Hmm. This is necessary for the test to be able to use it, but not sufficient: if a module is using "_dbus*" symbols, it needs the internal library (at least in Autotools-land, where we have symbol visibility).

Please commit this, but not the other patches from this bug right now. I'll put together a patch for the Autotools build system.
Comment 83 Simon McVittie 2013-08-29 09:47:35 UTC
Comment on attachment 81341 [details] [review]
[2] GetConnectionCredentials: add

applied
Comment 84 Simon McVittie 2013-08-29 09:47:54 UTC
Comment on attachment 81342 [details] [review]
[3] Document GetAdtAuditSessionData and  GetConnectionSELinuxSecurityContext

applied
Comment 85 Simon McVittie 2013-08-29 09:49:05 UTC
Comment on attachment 84802 [details] [review]
Assert when not seen WindowsSID on windows

This seems to be included in Attachment #84801 [details], so it's probably obsolete?

(I would have asked you to squash them into one commit anyway, tbh.)
Comment 86 Simon McVittie 2013-08-29 09:50:53 UTC
(In reply to comment #82)
> Export _dbus_getsid for test applications

This is included in Attachment #84801 [details] too?
Comment 87 Ralf Habacker 2013-08-29 09:56:08 UTC
Comment on attachment 84801 [details] [review]
Test for windows sid (update)

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

::: test/dbus-daemon.c
@@ +418,5 @@
> +          g_assert (!(seen & SEEN_WINDOWS_SID));
> +          g_assert_cmpuint (dbus_message_iter_get_arg_type (&var_iter), ==,
> +              DBUS_TYPE_STRING);
> +          dbus_message_iter_get_basic (&var_iter, &sid);
> +          g_message ("%s of this process is %s", name, sid);

here probably setting result to an initial value is missing.
Comment 88 Simon McVittie 2013-08-29 10:09:00 UTC
This no longer blocks Bug #47581: I've merged enough for that.
Comment 89 Ralf Habacker 2013-08-29 10:11:36 UTC
(In reply to comment #85)
> Comment on attachment 84802 [details] [review] [review]
> Assert when not seen WindowsSID on windows
> 
> This seems to be included in Attachment #84801 [details], so it's probably
> obsolete?
> 
> (I would have asked you to squash them into one commit anyway, tbh.)

yes, i made this patch obsolate
Comment 90 Ralf Habacker 2013-08-29 10:14:18 UTC
(In reply to comment #86)
> (In reply to comment #82)
> > Export _dbus_getsid for test applications
> 
> This is included in Attachment #84801 [details] too?

seems so, made it obsolate
Comment 91 Simon McVittie 2013-08-29 10:16:10 UTC
(In reply to comment #82)
> Please commit this, but not the other patches from this bug right now. I'll
> put together a patch for the Autotools build system.

Let's defer all of this until we have a test (without the credentials stuff) building and passing. I saw a patch from you somewhere which removed the --nofork on Windows, which AIUI is a prerequisite for any of these tests working on Windows - remind me, which bug is that on?
Comment 92 Ralf Habacker 2013-08-29 10:19:23 UTC
(In reply to comment #91)
> (In reply to comment #82)
> > Please commit this, but not the other patches from this bug right now. I'll
> > put together a patch for the Autotools build system.
> 
> Let's defer all of this until we have a test (without the credentials stuff)
> building and passing. I saw a patch from you somewhere which removed the
> --nofork on Windows, which AIUI is a prerequisite for any of these tests
> working on Windows - remind me, which bug is that on?

you are probably refering to https://bugs.freedesktop.org/attachment.cgi?id=84801

@@ -78,8 +81,12 @@ spawn_dbus_daemon (gchar *binary,
   gchar *argv[] = {
       binary,
       configuration,
+#ifdef DBUS_WIN
+      "--print-address", /* stdout */
+#else
       "--nofork",
       "--print-address=1", /* stdout */
+#endif
       NULL
Comment 93 Simon McVittie 2013-09-02 16:27:25 UTC
(In reply to comment #91)
> I saw a patch from you somewhere which removed the
> --nofork on Windows, which AIUI is a prerequisite for any of these tests
> working on Windows

Bug #68852 might make this unnecessary. I hope so.

(In reply to comment #82)
> if a module is using "_dbus*" symbols, it needs the internal
> library (at least in Autotools-land, where we have symbol visibility).

Bug #68852 should make it easier for the dbus-daemon test to be compiled against (dbus-glib and) libdbus-1.la on Unix, but libdbus-internal.la (so it can see _dbus_getsid()) on Windows.

I'd like to confirm that that test passes on Windows *without* the Bug #54445 stuff before I do that, though :-)
Comment 94 Simon McVittie 2013-09-03 11:09:21 UTC
With the patches from Bug #68852 merged, it should be possible to do something like this (untested):

if DBUS_WIN
test_dbus_daemon_CPPFLAGS = $(static_cppflags)
test_dbus_daemon_LDADD = libdbus-testutils-internal.la
else !DBUS_WIN
test_dbus_daemon_CPPFLAGS =
test_dbus_daemon_LDADD = \
    $(testutils_shared_if_possible) \
    $(GLIB_LIBS) \
    $(NULL)
endif !DBUS_WIN

to give test-dbus-daemon access to _dbus_getsid() on Windows, but link it dynamically (when possible) on Unix.
Comment 95 Simon McVittie 2013-09-27 11:27:28 UTC
For those who like Smack, SELinux or other environments with more credentials than just the POSIX ones: the OS-independent and POSIX parts of this are in git master, so now is the time to implement credentials-getting mechanisms for your favourite LSM or other credentials framework (please track as a separate bug, and use Attachment #75654 [details] as inspiration).

What's left on this bug is getting feature parity with the old credentials mechanisms on Windows, which is blocked by getting the regression test to work; that doesn't block Smack, SELinux, Solaris ADT etc.
Comment 96 Simon McVittie 2014-02-13 11:20:42 UTC
This is not Linux-specific, and is a feature enhancement, not a security vulnerability; removing security tag again.

If GetConnectionCredentials() can return incorrect information, *that* would be a security vulnerability. (If so, please email me privately.)
Comment 97 Tyler Hicks 2014-03-15 00:22:59 UTC
(In reply to comment #95)
> For those who like Smack, SELinux or other environments with more
> credentials than just the POSIX ones: the OS-independent and POSIX parts of
> this are in git master, so now is the time to implement credentials-getting
> mechanisms for your favourite LSM or other credentials framework (please
> track as a separate bug, and use Attachment #75654 [details] as inspiration).

Note that this WIP code has run-time errors. It attempts to append multiple bytes direcly to a variant container. What needs to happen is for the variant container to be opened, then an array container to be opened, and then the byte array to be appended. The middle step is missing from the WIP.

I've attached a patch to Bug #75113 that creates a new a{sv} helper called _dbus_asv_add_byte_array(). It may be of interest to anyone wanting to finish out this WIP code.
Comment 98 Simon McVittie 2015-01-26 19:24:59 UTC
Unassigning this, I'm not working on it.

I think the missing thing here is making the Windows test work, or possibly verifying that it already works.
Comment 99 Ralf Habacker 2015-02-10 15:25:31 UTC
Created attachment 113318 [details] [review]
Test for windows sid (rebased)
Comment 100 Ralf Habacker 2015-02-10 15:27:31 UTC
Created attachment 113319 [details] [review]
Minor optimization in _dbus_getsid()

This patch is required for attachment 113318 [details] [review].
Comment 101 Ralf Habacker 2015-02-10 15:42:56 UTC
With attachment 81389 [details] [review], attachment 113319 [details] [review], attachment 113318 [details] [review] applied I get from running test-dbus-daemon:

/creds:
** Message: ProcessID of this process is 76
** Message: WindowsSID of this process is S-1-5-21-0-0-0-1000
** Message: self sid S-1-5-21-0-0-0-1000 0
OK
/processid:
** Message: GetConnectionUnixProcessID returned 76
OK
/echo/session:
OK
/echo/limited:
OK
/no-reply/disconnect:
OK
/no-reply/timeout:
OK
/canonical-path/uae:
OK
Comment 102 Ralf Habacker 2015-02-10 15:44:21 UTC
> ** Message: self sid S-1-5-21-0-0-0-1000 0
expect this additional debug message not contained in the test case.
Comment 103 Simon McVittie 2015-02-10 17:46:28 UTC
Comment on attachment 113318 [details] [review]
Test for windows sid (rebased)

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

::: test/dbus-daemon.c
@@ +382,5 @@
> +          g_assert_cmpuint (dbus_message_iter_get_arg_type (&var_iter), ==,
> +              DBUS_TYPE_STRING);
> +          dbus_message_iter_get_basic (&var_iter, &sid);
> +          g_message ("%s of this process is %s", name, sid);
> +          if (_dbus_getsid (&self_sid, 0))

At the moment you can't call _dbus_getsid() unless the executable is statically linked to libdbus-internal, so if you want to do this, you will have to do something like:

if DBUS_WIN
# statically linked so we can call _dbus_getsid()
test_dbus_daemon_CPPFLAGS = $(static_cppflags)
test_dbus_daemon_LDADD = \
    libdbus-testutils-internal.la \
    $(GLIB_LIBS) \
    $(NULL)
else
test_dbus_daemon_CPPFLAGS = $(testutils_shared_if_possible_cppflags)
test_dbus_daemon_LDADD = \
    $(testutils_shared_if_possible_libs) \
    $(GLIB_LIBS) \
    $(NULL)
endif

(and maybe the CMake equivalent) - I would really prefer to keep as many tests as possible dynamically linked on Unix, because it makes them much more useful as integration tests.

Alternatively, if someone reviews Bug #83115 one day, then we can access _dbus functions even in dynamically-linked tests.

The other possibility is to put the comparison with _dbus_getsid in #ifdef DBUS_STATIC_BUILD.

@@ +384,5 @@
> +          dbus_message_iter_get_basic (&var_iter, &sid);
> +          g_message ("%s of this process is %s", name, sid);
> +          if (_dbus_getsid (&self_sid, 0))
> +              result = strcmp (self_sid, sid);
> +          g_assert_cmpuint (result, ==, 0);

No need for 'result', just use

g_assert_cmpstr (self_sid, ==, sid);

which does the same strcmp internally, but also gives a better assertion-failure message if it fails.

I think you're leaking self_sid here? Please dbus_free() it.
Comment 104 Simon McVittie 2015-02-10 17:48:49 UTC
Comment on attachment 113319 [details] [review]
Minor optimization in _dbus_getsid()

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

::: dbus/dbus-sysdeps-win.c
@@ +993,5 @@
>    PSID psid;
>    int retval = FALSE;
>  
> +  HANDLE process_handle = process_id == 0 ? GetCurrentProcess()
> +          : OpenProcess(is_winxp_sp3_or_lower() ? PROCESS_QUERY_INFORMATION : PROCESS_QUERY_LIMITED_INFORMATION, FALSE, process_id);

This seems like too many ?: operators :-)

I think it'd be a lot clearer like this:

HANDLE process_handle;

if (process_id == 0)
  process_handle = GetCurrentProcess ();
else if (is_winxp_sp3_or_lower ())
  process_handle = OpenProcess (PROCESS_QUERY_INFORMATION, FALSE, process_id);
else
  process_handle = OpenProcess (PROCESS_QUERY_LIMITED_INFORMATION, FALSE, process_id);
Comment 105 Ralf Habacker 2015-02-10 17:57:31 UTC
Created attachment 113321 [details] [review]
Minor optimization in _dbus_getsid() (update)

Made if else chain easier
Comment 106 Simon McVittie 2015-02-10 18:05:51 UTC
Comment on attachment 113321 [details] [review]
Minor optimization in _dbus_getsid() (update)

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

Much better :-)
Comment 107 Simon McVittie 2015-02-10 18:08:41 UTC
(In reply to Simon McVittie from comment #103)
> I would really prefer to keep as many
> tests as possible dynamically linked on Unix

Clarification: on Unix with Autotools. I don't mind if some or all of the tests are consistently statically-linked under CMake if that makes your life easier.
Comment 108 Simon McVittie 2015-02-10 20:44:34 UTC
(In reply to Simon McVittie from comment #103)
> Alternatively, if someone reviews Bug #83115 one day, then we can access
> _dbus functions even in dynamically-linked tests.

Given that this touches the entire build system and has 4 months' worth of merge conflicts, I would recommend doing one of the others for now, and we can clean it up if Bug #83115 is ever merged.

I should have known this would happen when I worked on Bug #83115 :-(
Comment 109 Ralf Habacker 2015-02-10 22:51:17 UTC
Created attachment 113330 [details] [review]
Add test for Windows SID (update)

- fix leak
- use g_assert_cmpstr
Comment 110 Ralf Habacker 2015-02-10 22:53:40 UTC
(In reply to Simon McVittie from comment #103)
> Comment on attachment 113318 [details] [review] [review]
> Test for windows sid (rebased)
> 
> Review of attachment 113318 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: test/dbus-daemon.c
> @@ +382,5 @@
> > +          g_assert_cmpuint (dbus_message_iter_get_arg_type (&var_iter), ==,
> > +              DBUS_TYPE_STRING);
> > +          dbus_message_iter_get_basic (&var_iter, &sid);
> > +          g_message ("%s of this process is %s", name, sid);
> > +          if (_dbus_getsid (&self_sid, 0))
> 
> At the moment you can't call _dbus_getsid() unless the executable is
> statically linked to libdbus-internal, 
We are using it currently only in a bus not installed test application, so no problem here.

> No need for 'result', just use
> 
> g_assert_cmpstr (self_sid, ==, sid);
fixed 
> I think you're leaking self_sid here? Please dbus_free() it.
sure, but need to use LocalFree().
Comment 111 Simon McVittie 2015-02-11 11:48:05 UTC
(In reply to Ralf Habacker from comment #110)
> > At the moment you can't call _dbus_getsid() unless the executable is
> > statically linked to libdbus-internal, 
>
> We are using it currently only in a bus not installed test application, so
> no problem here.

test-dbus-daemon (dbus-daemon.c) is built dynamically-linked under Autotools, and optionally installed as an integration test on Unix.
Comment 112 Simon McVittie 2015-02-11 11:59:26 UTC
(In reply to Simon McVittie from comment #111)
> test-dbus-daemon (dbus-daemon.c) is built dynamically-linked under
> Autotools, and optionally installed as an integration test on Unix.

Actually, it's statically linked to libdbus-internal if you don't have dbus-glib, which people compiling for Windows probably don't... so your patch is theoretically not right in all situations, but it's good enough in practice.

Fixed in git for 1.9.12, thanks! (I pushed it myself rather than waiting for you, because I wanted to avoid conflicts when I work on Bug #89041, which will also touch test-dbus-daemon.)
Comment 113 Simon McVittie 2015-02-11 14:24:59 UTC
Created attachment 113360 [details] [review]
bus_driver_handle_get_connection_credentials: do not  assert on OOM

dbus_connection_get_windows_user is documented to return TRUE but
put NULL in its argument if OOM is reached.

---

Happened to spot this while adapting this code for the Linux security label (also a string) on Bug #89041. Yes, it's a weird calling convention, but it's what the docs say.
Comment 114 Ralf Habacker 2015-02-11 15:28:58 UTC
Comment on attachment 113360 [details] [review]
bus_driver_handle_get_connection_credentials: do not  assert on OOM

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

looks good - I already committed to master


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.