|
Description
Simon McVittie
2013-03-04 12:29:57 UTC
(In reply to comment #0) > On Bug #54445 I wrote: > > > (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). Exactly. in _dbus_read_credentials_socket() there is called later ... _dbus_credentials_add_from_current_process (credentials); dbus_bool_t _dbus_read_credentials_socket (int handle, DBusCredentials *credentials, DBusError *error) { int bytes_read = 0; DBusString buf; // could fail due too OOM if (_dbus_string_init(&buf)) { bytes_read = _dbus_read_socket(handle, &buf, 1 ); if (bytes_read > 0) _dbus_verbose("got one zero byte from server"); _dbus_string_free(&buf); } _dbus_credentials_add_from_current_process (credentials); _dbus_verbose("FIXME: get faked credentials from current process"); return TRUE; } > > What should happen is more like this: > > * _dbus_read_credentials_socket() should read the byte, this is already done, see above. > but not add any credentials, because on Windows, you can't tell > who is at the other end of a socket. > (Unless you can, in which case, do that.) At least for local host connections with the help of GetExtendedTypTable() http://msdn.microsoft.com/en-us/library/windows/desktop/aa365928%28v=vs.85%29.aspx it is possible to get the ports and pid's of all dbus-daemon tcp connections (marked with '*') 0100007f 62617 00000000 0 8 * listen 0100007f 39412 00000000 0 8 * listen 0100007f 39412 0100007f 24546 8 * 0100007f 24802 0100007f 39412 69 (2. dbus-monitor) 0100007f 24546 0100007f 39412 55 (1. dbus-monitor) 0100007f 39412 0100007f 24802 8 * From the pid the process owner (sid) could be fetched. There is still the question how to identify the related client in the list. > * If a peer has authenticated itself with a SASL mechanism that can > reasonably be expected to prove its identity - of which the only one > we support on Windows is DBUS_COOKIE_SHA1, which demonstrates the > ability to write to the user's home directory - then it should pick up > the SID that way. It should not pick up the process ID, because you > can't tell the process ID like this. > > * If a peer has authenticated itself using SASL ANONYMOUS > (disabled by default), it should not have any credentials. If it would be possible to get the peers connection port from the socket descriptor, the pid and sid are fetchable in any case as mentioned above. (In reply to comment #1) > > * If a peer has authenticated itself with a SASL mechanism that can > > reasonably be expected to prove its identity [...] then it should pick up > > the SID that way. Actually, that's not this function. On Unix, the information from this function is only used if you're doing SASL EXTERNAL, to prove that you're who you say you are; Unix will always return "I have no idea" from the equivalent function for TCP sockets (and then if the peer authenticates with DBUS_COOKIE_SHA1, we'll use the authenticated uid from that instead). So, on Windows, a minimal implementation of this function would be to return "I don't know" (no information); and in the "I don't know" case, another part of libdbus should fill in the SID. What happens if you just delete the _dbus_credentials_add_from_current_process() call, which is the part that's highly misleading? Assuming Windows uses DBUS_COOKIE_SHA1 authentication, it should continue to work OK and provide the SID (only). Of course, ideally, more information would be better: > > but not add any credentials, because on Windows, you can't tell > > who is at the other end of a socket. > > (Unless you can, in which case, do that.) > > At least for local host connections with the help of GetExtendedTypTable() > http://msdn.microsoft.com/en-us/library/windows/desktop/aa365928%28v=vs. > 85%29.aspx it is possible to get the ports and pid's of all dbus-daemon tcp > connections (marked with '*') Great! In that case, we could have had SASL EXTERNAL authentication on Windows all along... (This is roughly the C API equivalent of Linux's netstat tool, right?) > 0100007f 62617 00000000 0 8 * listen > 0100007f 39412 00000000 0 8 * listen > 0100007f 39412 0100007f 24546 8 * > 0100007f 24802 0100007f 39412 69 (2. dbus-monitor) > 0100007f 24546 0100007f 39412 55 (1. dbus-monitor) > 0100007f 39412 0100007f 24802 8 * > > From the pid the process owner (sid) could be fetched. There is still the > question how to identify the related client in the list. dbus-daemon can call getpeername() on the socket to identify the other end (in this case 127.0.0.1:24802 and 127.0.0.1:24546, right?), then if it's an address of type AF_INET (IPv4) and the address is 127.0.0.1, look it up in the table. 55 and 69 are dbus-monitor process IDs here, right? Then presumably you can go from PID 55 to the corresponding SID relatively easily. Created attachment 76052 [details] [review] Retrieve pid and sid from tcp connection These patches are a followup of https://bugs.freedesktop.org/show_bug.cgi?id=54445#c50 Created attachment 76053 [details] [review] Mingw plattform fixes Created attachment 76054 [details] [review] some fixes - fixed comparing ipv4 localhost - get_peer_pid_from_tcp_handle(): disable ipv6 handling because ipv6 do not works currently - _dbus_read_credentials_socket(): do not return error when not getting peers pid Created attachment 76098 [details] [review] GetTcpTable2() isn't available in msvc windows XP headers (indicated by using _WIN32_WINNT 0x0501). Created attachment 76099 [details] [review] fixed related debug mesage eol Created attachment 76100 [details] [review] Make _dbus_credentials_add_pid() plattform independent. (In reply to comment #50) > > + getpeername(handle, (struct sockaddr*)&addr, &len); > > coding style: getpeername (handle, (struct sockaddr *) &addr...); > > (and many more instances of the same things) this is very hard to follow when not working with dbus code every day. We really need an indent or similar setting, which would makes it much easier. Simply run indent ... and most of the style changes are ready. > 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; > } There are several places where goto's are used 28 again: 62 default: 3 done: 1 end: 1 err: 18 error: 5 fail: 60 failed: 2 failure: 6 finish: 2 finished: 1 invalid: 2 lose: 9 next: 2 noconv: 13 nomem: 46 oom: 114 out: 5 restart: 4 retry: 1 unlock: Do you have ever thought about some basic refactoring, which would make this goto thing mostly obsolate for example class DBusSID { public: bool fromPID(dbus_pid_t pid); }; void function() { ... DBusSID a; a.fromPID(apid); ... if (error) return true; else return false; // DBusSID instance is cleanup automatically in the destructor } Comment on attachment 76052 [details] [review] Retrieve pid and sid from tcp connection Review of attachment 76052 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-sysdeps-win.c @@ +117,5 @@ > + dbus_pid_t result; > + DWORD size; > + MIB_TCPTABLE2 *tcp_table; > + int i; > + int localhost; maybe use dbus_bool_t for this? @@ +126,5 @@ > + if (addr.ss_family == AF_INET) > + { > + struct sockaddr_in *s = (struct sockaddr_in *)&addr; > + peer_port = ntohs (s->sin_port); > + localhost = s->sin_addr.s_addr == INADDR_LOOPBACK; I'd prefer some extra parentheses to make this a little clearer: localhost = (s->sin_addr.s_addr == INADDR_LOOPBACK); (and similar for the IPv6 case) @@ +129,5 @@ > + peer_port = ntohs (s->sin_port); > + localhost = s->sin_addr.s_addr == INADDR_LOOPBACK; > + } > + else > + { /* AF_INET6 */ else if (addr.ss_family == AF_INET6) { ... this ... } else { _dbus_verbose ("no idea what address family %d is", addr.ss_family); return 0; } Comment on attachment 76053 [details] [review] Mingw plattform fixes Review of attachment 76053 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-sysdeps-win.c @@ +60,5 @@ > /* Declarations missing in mingw's headers */ > extern BOOL WINAPI ConvertStringSidToSidA (LPCSTR StringSid, PSID *Sid); > extern BOOL WINAPI ConvertSidToStringSidA (PSID Sid, LPSTR *StringSid); > > +#ifndef _MSC_VER On mingw, I think this is going to fail to compile if _WIN32_WINNT is set to a version that *does* declare them. Can we just set _WIN32_WINNT (that's the "target Windows version", right?) to a "new enough" version (I think that means Vista)? Or do you still support XP? @@ +84,5 @@ > + DWORD dwNumEntries; > + MIB_TCPROW2 table[ANY_SIZE]; > +} MIB_TCPTABLE2,*PMIB_TCPTABLE2; > + > +ULONG WINAPI GetTcpTable2( As far as I understand it, the purpose of the _WIN32_WINNT stuff is to avoid using symbols that aren't available on all supported versions of Windows. Is that true? If we call GetTcpTable2, isn't that going to mean that on XP, applications using libdbus will just fail to start up, because the required symbol isn't found in the relevant DLL? As far as I understand it, the standard trick for "conditional functionality" is to use GetProcAddress() to see whether it exists at runtime... Comment on attachment 76054 [details] [review] some fixes Review of attachment 76054 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-sysdeps-win.c @@ +1815,4 @@ > pid = get_peer_pid_from_tcp_handle (handle); > > if (pid == 0) > + return TRUE; Ah yes, this was wrong before (and I missed that while reviewing). Please include this 1-line change in the first patch in the series. Comment on attachment 76099 [details] [review] fixed related debug mesage eol Review of attachment 76099 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-sysdeps-win.c @@ +1807,4 @@ > bytes_read = _dbus_read_socket(handle, &buf, 1 ); > > if (bytes_read > 0) > + _dbus_verbose("got one zero byte from server\n"); Fine, feel free to apply this "ahead". (In reply to comment #9) > this is very hard to follow when not working with dbus code every day. We > really need an indent or similar setting, which would makes it much easier. > Simply run indent ... and most of the style changes are ready. "I know it when I see it" ;-) It's basically Gtk style, which is similar to GNU style but without "hard" tabs, if that helps... I don't know a set of indent options that produces Gtk style, though. ("indent -nut" is probably a good start.) > There are several places where goto's are used Right, for different purposes. "again" is for EAGAIN handling, "default" is actually a case label rather than a goto label, "out" is like C++'s finally, and it seems "oom" and "failed" are our conventional equivalents for C++'s catch. > class DBusSID { No, libdbus is not going to be converted into C++. C is the lowest-common-denominator language for system libraries, with a stable ABI per platform; C++ needs an extra runtime library and has ABI concerns. (In situations where C++ is acceptable, I'd recommend GDBus - which is not actually C++ but has a similar "weight" of runtime libraries and assumptions - or Qt.) Comment on attachment 76100 [details] [review] Make _dbus_credentials_add_pid() plattform independent. Review of attachment 76100 [details] [review]: ----------------------------------------------------------------- Is dbus_pid_t guaranteed to be big enough for Windows process IDs? Looking at the source code, dbus_pid_t appears to be unsigned long, and GetCurrentProcessId() returns a DWORD; so I think we're fine there. ISO C says a long is at least 32 bits, and AIUI DWORD is always 32-bit unsigned? This would benefit from a second line of commit message that says something like "Windows also has numeric process IDs that fit in an unsigned long, so there's no reason this has to be Unix-specific". (In reply to comment #2) > So, on Windows, a minimal implementation of this function would be to return > "I don't know" (no information); and in the "I don't know" case, another > part of libdbus should fill in the SID. Attachment #76099 [details] "fixed related debug mesage eol" is fine to apply immediately. While we work out whether GetTcpTable2() is portable to all Windows versions that you want to support... could you please see what happens if you just delete the call to _dbus_credentials_add_from_current_process() (and the _dbus_verbose that follows) from _dbus_read_credentials_socket()? That would fix the original reason I opened this bug - that function providing information that is untrue - and if I understand correctly, user equivalence will be authenticated via DBUS_COOKIE_SHA1, just like it is for TCP sockets on Unix. That way, you'd have a SID but no PID. Then, if you can work out how to reconcile the GetTcpTable2() stuff with Windows-version-compatibility, we can do that part later (providing proper support for EXTERNAL authentication, as used over Unix sockets on Unix). Comment on attachment 76100 [details] [review] Make _dbus_credentials_add_pid() plattform independent. Review of attachment 76100 [details] [review]: ----------------------------------------------------------------- This is incomplete, and will break the build on Unix if applied as-is - there are a couple of calls to ...add_unix_pid() in dbus-sysdeps-unix.c which would also need changing. If you're going to make this change, you should probably also change ...get_unix_pid(), and the unix_pid struct member, to just say "pid" too :-) dbus_connection_get_unix_process_id() can't be renamed because it's in the public API, but you could add dbus_connection_get_process_id() (returning the same thing) if you want. Similarly, the GetConnectionUnixProcessID D-Bus method can't be renamed, because it's API. I would be inclined to not add a GetConnectionProcessID, because GetConnectionCredentials is better anyway. (In reply to comment #13) > Comment on attachment 76099 [details] [review] [review] > fixed related debug mesage eol > > Review of attachment 76099 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: dbus/dbus-sysdeps-win.c > @@ +1807,4 @@ > > bytes_read = _dbus_read_socket(handle, &buf, 1 ); > > > > if (bytes_read > 0) > > + _dbus_verbose("got one zero byte from server\n"); > > Fine, feel free to apply this "ahead". done (In reply to comment #16) > (In reply to comment #2) > > So, on Windows, a minimal implementation of this function would be to return > > "I don't know" (no information); and in the "I don't know" case, another > > part of libdbus should fill in the SID. > > Attachment #76099 [details] "fixed related debug mesage eol" is fine to > apply immediately. > > While we work out whether GetTcpTable2() is portable to all Windows versions > that you want to support... could you please see what happens if you just > delete the call to _dbus_credentials_add_from_current_process() (and the > _dbus_verbose that follows) from _dbus_read_credentials_socket()? > > That would fix the original reason I opened this bug - that function > providing information that is untrue will send a separate patch > - and if I understand correctly, user > equivalence will be authenticated via DBUS_COOKIE_SHA1, just like it is for > TCP sockets on Unix. That way, you'd have a SID but no PID. I have to dig into > > Then, if you can work out how to reconcile the GetTcpTable2() stuff with > Windows-version-compatibility, we can do that part later (providing proper > support for EXTERNAL authentication, as used over Unix sockets on Unix). in this bug or in a different bug ? Created attachment 76149 [details] [review] Do not retrieve credential information from the wrong side of the connection. (In reply to comment #17) > Comment on attachment 76100 [details] [review] [review] > Make _dbus_credentials_add_pid() plattform independent. > > Review of attachment 76100 [details] [review] [review]: > ----------------------------------------------------------------- > > This is incomplete, and will break the build on Unix if applied as-is - > there are a couple of calls to ...add_unix_pid() in dbus-sysdeps-unix.c > which would also need changing. ups, I used the configured project in qtcreator and did the replace on the project level not on the file level. Will fix. > > If you're going to make this change, you should probably also change > ...get_unix_pid(), and the unix_pid struct member, to just say "pid" too :-) I will prepare a patch containing both changes. > dbus_connection_get_unix_process_id() can't be renamed because it's in the > public API, > but you could add dbus_connection_get_process_id() (returning > the same thing) if you want. Would not be adding dbus_connection_get_process_id() dbus_connection_get_process_user() and mark dbus_connection_get_unix_process_id() dbus_connection_get_unix_user() as deprecated and remove it in dbus-2.0 as good solution ? > Similarly, the GetConnectionUnixProcessID D-Bus method can't be renamed, > because it's API. I would be inclined to not add a GetConnectionProcessID, > because GetConnectionCredentials is better anyway. Similar to the dbus interface. Mark GetConnectionUnixUser GetConnectionUnixProcessID as deprecated and remove it in dbus 2.0 (In reply to comment #15) > Comment on attachment 76100 [details] [review] [review] > Make _dbus_credentials_add_pid() plattform independent. > > Review of attachment 76100 [details] [review] [review]: > ----------------------------------------------------------------- > > Is dbus_pid_t guaranteed to be big enough for Windows process IDs? > > Looking at the source code, dbus_pid_t appears to be unsigned long, and > GetCurrentProcessId() returns a DWORD; so I think we're fine there. ISO C > says a long is at least 32 bits, and AIUI DWORD is always 32-bit unsigned? yes, http://msdn.microsoft.com/en-us/library/cc230318.aspx > This would benefit from a second line of commit message that says something > like "Windows also has numeric process IDs that fit in an unsigned long, so > there's no reason this has to be Unix-specific". okay Created attachment 76157 [details] [review] Refactored to use dbus_pid_t instead of unsigned long. Created attachment 76158 [details] [review] Rename the term 'unix_pid' to 'pid' in variables and functions. Created attachment 76159 [details] [review] Use dbus_pid_t instead of unsigned long. fixed message (In reply to comment #19) > > Then, if you can work out how to reconcile the GetTcpTable2() stuff with > > Windows-version-compatibility, we can do that part later (providing proper > > support for EXTERNAL authentication, as used over Unix sockets on Unix). > > in this bug or in a different bug ? I don't mind. (In reply to comment #21) > Would not be adding > > dbus_connection_get_process_id() > dbus_connection_get_process_user() > > and mark > > dbus_connection_get_unix_process_id() > dbus_connection_get_unix_user() > > as deprecated and remove it in dbus-2.0 as good solution ? I would rather not mark the ..._unix_... versions as deprecated until we have had a stable release where the other versions exist. It's not as if it'll be any extra code to have both. > Similar to the dbus interface. Mark > > GetConnectionUnixUser > GetConnectionUnixProcessID > > as deprecated and remove it in dbus 2.0 I'm not sure that there will ever be a D-Bus 2.0, but when GetConnectionCredentials lands, yes we can flag these as deprecated - that's the purpose of GetConnectionCredentials. Comment on attachment 76149 [details] [review] Do not retrieve credential information from the wrong side of the connection. Review of attachment 76149 [details] [review]: ----------------------------------------------------------------- Needs a newline at the end of the string, I think? Does it work? If your dbus-daemon has this patch, is DBUS_COOKIE_SHA1 enough to get session bus users authenticated? If it is, add the newline and ship it :-) Comment on attachment 76158 [details] [review] Rename the term 'unix_pid' to 'pid' in variables and functions. Review of attachment 76158 [details] [review]: ----------------------------------------------------------------- Looks good. Comment on attachment 76159 [details] [review] Use dbus_pid_t instead of unsigned long. Review of attachment 76159 [details] [review]: ----------------------------------------------------------------- I can see why you'd want this, but no, this is not OK. At the moment, dbus_pid_t is internal to dbus. It's at least as large as an unsigned long, but we could make it larger if we needed to (if we encounter a platform with 32-bit long but 64-bit pids) and that wouldn't be an ABI break. Admittedly, dbus_connection_get_unix_process_id() wouldn't work very well on such a platform, but it would at least be able to return FALSE for large pids; we could add a dbus_connection_get_large_process_id() with a larger "out" parameter, if we really needed one. By contrast, moving dbus_pid_t into an externally-visible header would freeze it at "same size as an unsigned long" forever (or until our next ABI break, which in practice is basically forever). ::: dbus/dbus-connection.h @@ +27,4 @@ > #ifndef DBUS_CONNECTION_H > #define DBUS_CONNECTION_H > > +#include <dbus/dbus-sysdeps.h> dbus-sysdeps.h is not a public or installed header, so it cannot be included by a public or installed header. Created attachment 76166 [details] [review] Add function get_peer_pid_from_tcp_handle() which returns pid and sid from tcp connection peer. fixed win api includes problem by using GetExtendedTcpTable() (In reply to comment #27) > Comment on attachment 76149 [details] [review] [review] > Do not retrieve credential information from the wrong side of the connection. > > Review of attachment 76149 [details] [review] [review]: > ----------------------------------------------------------------- > > Needs a newline at the end of the string, I think? >ha > Does it work? If your dbus-daemon has this patch, is DBUS_COOKIE_SHA1 enough > to get session bus users authenticated? If it is, add the newline and ship > it :-) I think it is: 1. added <auth>DBUS_COOKIE_SHA1</auth> to session.conf 2. started dbus-daemon 3. connected with dbus-monitor in the dbus verbose log I found 55: [dbus/dbus-auth.c(1704):handle_auth] server: Trying mechanism DBUS_COOKIE_SHA1 55: [dbus/dbus-transport.c(655):auth_via_default_rules] Client authorized as SID 'S-1-5-21-0-0-0-1000'matching our SID 'S-1-5-21-0-0-0-1000' which looks like what you expected. Created attachment 76195 [details] [review] Further debug message eol fix Created attachment 76197 [details] [review] Use table class which has been implemented in wine 1.5.3. Created attachment 76198 [details] [review] Add debug message for returning from get_peer_pid_from_tcp_handle(). (In reply to comment #28) > Comment on attachment 76158 [details] [review] [review] > Rename the term 'unix_pid' to 'pid' in variables and functions. > > Review of attachment 76158 [details] [review] [review]: > ----------------------------------------------------------------- > > Looks good. applied Comment on attachment 76195 [details] [review] Further debug message eol fix Review of attachment 76195 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 76197 [details] [review] Use table class which has been implemented in wine 1.5.3. Review of attachment 76197 [details] [review]: ----------------------------------------------------------------- Please clarify why this is a good thing. Some possible things you might have meant include: "there is no Wine version that implements TCP_TABLE_OWNER_PID_CONNECTIONS, but Wine >= 1.5.3 implements TCP_TABLE_OWNER_PID_ALL, so use ALL to make this testable under Wine" "TCP_TABLE_OWNER_PID_ALL is better than TCP_TABLE_OWNER_PID_CONNECTIONS because [...], and we don't need to avoid ALL for the benefit of testing under Wine, because Wine has implemented it since 1.5.3" Comment on attachment 76198 [details] [review] Add debug message for returning from get_peer_pid_from_tcp_handle(). Review of attachment 76198 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-sysdeps-win.c @@ +187,4 @@ > result = p->dwOwningPid; > } > > + _dbus_verbose ("got pid %d\n", result); Sure, if this is useful information to have... (In reply to comment #31) > I think it is: > 1. added <auth>DBUS_COOKIE_SHA1</auth> to session.conf > 2. started dbus-daemon > 3. connected with dbus-monitor Just to confirm, if session.conf does not include <auth>DBUS_COOKIE_SHA1</auth>, does connecting a dbus-monitor to a dbus-daemon still work? (Hopefully it does, because the default if there is no <auth/> is to try all possible mechanisms, I think.) (In reply to comment #39) > (In reply to comment #31) > > I think it is: > > 1. added <auth>DBUS_COOKIE_SHA1</auth> to session.conf > > 2. started dbus-daemon > > 3. connected with dbus-monitor > > Just to confirm, if session.conf does not include > <auth>DBUS_COOKIE_SHA1</auth>, does connecting a dbus-monitor to a > dbus-daemon still work? > > (Hopefully it does, because the default if there is no <auth/> is to try all > possible mechanisms, I think.) yes, this is done in static dbus_bool_t auth_via_default_rules (DBusTransport *transport) { .... /* By default, connection is allowed if the client is 1) root or 2) * has the same UID as us or 3) anonymous is allowed. */ on windows case 1 and 3 applies. (In reply to comment #38) > Comment on attachment 76198 [details] [review] [review] > Add debug message for returning from get_peer_pid_from_tcp_handle(). > > Review of attachment 76198 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: dbus/dbus-sysdeps-win.c > @@ +187,4 @@ > > result = p->dwOwningPid; > > } > > > > + _dbus_verbose ("got pid %d\n", result); > > Sure, if this is useful information to have... yes this makes debugging easier. :-) Any problem to combine - Further debug message eol fix - Use table class which has been implemented in wine 1.5.3 - Add debug message for returning from get_peer_pid_from_tcp_handle and - Add function get_peer_pid_from_tcp_handle() which returns pid and sid from tcp connection peer into one commit ? I would add the wine related note. (In reply to comment #41) > Any problem to combine > > - Further debug message eol fix > - Use table class which has been implemented in wine 1.5.3 > - Add debug message for returning from get_peer_pid_from_tcp_handle > > and > - Add function get_peer_pid_from_tcp_handle() which returns pid and sid from > tcp connection peer > > into one commit ? That's fine, but please put that commit (including the note about why you chose that table) up for review. Created attachment 76342 [details] [review] Add function get_peer_pid_from_tcp_handle() which returns pid and sid from tcp connection peer (combined) (In reply to comment #43) > Created attachment 76342 [details] [review] [review] > Add function get_peer_pid_from_tcp_handle() which returns pid and sid from > tcp connection peer (combined) Hi Simon, any time for review ? Thanks :-) Comment on attachment 76342 [details] [review] Add function get_peer_pid_from_tcp_handle() which returns pid and sid from tcp connection peer (combined) Review of attachment 76342 [details] [review]: ----------------------------------------------------------------- Looks good, just a couple of things: ::: dbus/dbus-sysdeps-win.c @@ +109,5 @@ > + * @param handle tcp socket descriptor > + * @return process id or 0 in case the process id could not be fetched > + */ > +dbus_pid_t > +get_peer_pid_from_tcp_handle (int handle) This function should be static (or prefixed with _dbus_ if it's used outside this file). @@ +178,5 @@ > + return 0; > + } > + > + result = 0; > + for (i = 0; i < (int) tcp_table->dwNumEntries; i++) Make i a DWORD or whatever the appropriate type is, then you won't need a cast? Created attachment 77431 [details] [review] updated Comment on attachment 77431 [details] [review] updated > > Looks good, just a couple of things: > fixed and applied 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.