DBus version 1.7.2 Windows XP sp3 (32-bit) The change at dbus-sysdeps-win.c:845 makes the session daemon exit with an error message: "OpenProcessToken failed: Unknown error code 5 or FormatMessage failed". Previous version (1.7.0) works fine. The change is in http://cgit.freedesktop.org/dbus/dbus/commit/?id=8159956ed4d34ff217f67758a72005fe4362aa45
Ralf, do you support Windows XP, or is this WONTFIX?
+ HANDLE process_handle = OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION, FALSE, process_id); + + if (!OpenProcessToken (process_handle, TOKEN_QUERY, &process_token)) { _dbus_win_warn_win_error ("OpenProcessToken failed", GetLastError ()); Now that libdbus checks processes' credentials better, this code path might have to be recoverable (_dbus_verbose() or something) instead of a fatal-by-default warning: we don't want the dbus-daemon to crash whenever a process that it can't inspect connects to it. OpenProcess() should probably also have error-checking. According to MSDN, XP doesn't have PROCESS_QUERY_LIMITED_INFORMATION, only PROCESS_QUERY_INFORMATION. Maybe use P_Q_I instead of P_Q_L_I if VerifyVersionInfo() says the version is less than 6.0?
Created attachment 82667 [details] [review] add winxp sp3 support
Created attachment 82670 [details] dump sid on return of dbus_getsid()
(In reply to comment #3) > Created attachment 82667 [details] [review] [review] > add winxp sp3 support Just verified that this patch on Windows XP.
Patch looks good then, thanks!
(In reply to comment #6) > Patch looks good then, thanks! will apply. There seems to be an additional "not-available-on-windows-xp" bug: dbus-daemon: [dbus/dbus-sysdeps-win.c(176):_dbus_get_peer_pid_from_tcp_handle] Error fetching tcp table 87 caused by if ((result = GetExtendedTcpTable (tcp_table, &size, TRUE, AF_INET, TCP_TABLE_OWNER_PID_ALL, 0)) != NO_ERROR)
Created attachment 83797 [details] [review] Fixed bug of unsupported GetExtendedTcpTable() table option on Windows XP SP2 and earlier
Comment on attachment 83797 [details] [review] Fixed bug of unsupported GetExtendedTcpTable() table option on Windows XP SP2 and earlier Review of attachment 83797 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-sysdeps-win.c @@ +113,5 @@ > + DWORD dwLocalPort; > + DWORD dwRemoteAddr; > + DWORD dwRemotePort; > + DWORD dwProcessId; > +} MIB_TCPROW_EX, *PMIB_TCPROW_EX; Isn't there a system header you can get this from? If we do need to declare it locally because of OS or SDK bugs, please give a comment with a reference to whatever MS documentation tells us it looks like this (MSDN URL or something); or if it isn't documented by MS, some sort of reference for where you found this. @@ +119,5 @@ > +typedef struct _MIB_TCPTABLE_EX > +{ > + DWORD dwNumEntries; > + MIB_TCPROW_EX table[ANY_SIZE]; > +} MIB_TCPTABLE_EX, *PMIB_TCPTABLE_EX; Likewise. @@ +141,5 @@ > + HMODULE hModule = LoadLibrary ("iphlpapi.dll"); > + if (hModule == NULL) > + return FALSE; > + > + // XP and later Do we support anything older than XP? If not, can't you just define WINNT_VERSION (or whatever it's called) to a version that will declare/link this function in the usual way? @@ +229,5 @@ > + > + for (dwSize = 0; dwSize < lpBuffer->dwNumEntries; dwSize++) > + { > + int local_port = ntohs (lpBuffer->table[dwSize].dwLocalPort); > + if (local_port == peer_port) Isn't dwLocalPort the dbus-daemon end, and dwRemotePort the peer (other process) end? That's what I'd expect from the naming, anyway. Don't you need to check that dwRemoteAddr is local, too? I suppose dwProcessId might be guaranteed to be 0 for remote connections, but still.
(In reply to comment #9) > Comment on attachment 83797 [details] [review] [review] > Fixed bug of unsupported GetExtendedTcpTable() table option on Windows XP > SP2 and earlier > > Review of attachment 83797 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: dbus/dbus-sysdeps-win.c > @@ +113,5 @@ > > + DWORD dwLocalPort; > > + DWORD dwRemoteAddr; > > + DWORD dwRemotePort; > > + DWORD dwProcessId; > > +} MIB_TCPROW_EX, *PMIB_TCPROW_EX; > > Isn't there a system header you can get this from? no, the related windows api function mentioned at http://msdn.microsoft.com/en-us/library/windows/desktop/aa365804%28v=vs.85%29.aspx use a PVOID. > > If we do need to declare it locally because of OS or SDK bugs, please give a > comment with a reference to whatever MS documentation tells us it looks like > this (MSDN URL or something); not available > or if it isn't documented by MS, some sort of reference for where you found this. will add http://forums.codeguru.com/showthread.php?188092-Enumerating-TCP-ports-and-mapping-them-to-PID-%28for-XP%29 > > @@ +119,5 @@ > > +typedef struct _MIB_TCPTABLE_EX > > +{ > > + DWORD dwNumEntries; > > + MIB_TCPROW_EX table[ANY_SIZE]; > > +} MIB_TCPTABLE_EX, *PMIB_TCPTABLE_EX; > > Likewise. > > @@ +141,5 @@ > > + HMODULE hModule = LoadLibrary ("iphlpapi.dll"); > > + if (hModule == NULL) > > + return FALSE; > > > > + // XP and later > > Do we support anything older than XP? no > If not, can't you just define WINNT_VERSION (or whatever it's called) to a > version that will declare/link this function in the usual way? no, its undefined in the import library. ld .... -lwinspool -lshell32 -lole32 -loleaut32 -luuid -lcomdlg32 -ladvapi32 CMakeFiles/dbus-1.dir/objects.a(dbus-sysdeps-win.obj):dbus-sysdeps-win.c:(.text+0x2e7): undefined reference to `AllocateAndGetTcpExTableFromStack@0' dlls/iphlpapi/ipstats.h:36:DWORD WINAPI AllocateAndGetTcpTableFromStack(PMIB_TCPTABLE *ppTcpTable, BOOL bOrder, HANDLE heap, DWORD flags) DECLSPEC_HIDDEN; include/winnt.h:160:# define DECLSPEC_HIDDEN __attribute__((visibility ("hidden"))) need to stay with LoadLibrary() > > @@ +229,5 @@ > > + > > + for (dwSize = 0; dwSize < lpBuffer->dwNumEntries; dwSize++) > > + { > > + int local_port = ntohs (lpBuffer->table[dwSize].dwLocalPort); > > + if (local_port == peer_port) > > Isn't dwLocalPort the dbus-daemon end, and dwRemotePort the peer (other > process) end? That's what I'd expect from the naming, anyway. Previously working version uses dwLocalPort too http://cgit.freedesktop.org/dbus/dbus/tree/dbus/dbus-sysdeps-win.c#n185. Will double check this. > Don't you need to check that dwRemoteAddr is local, too? This has been checked earlier in the function see http://cgit.freedesktop.org/dbus/dbus/tree/dbus/dbus-sysdeps-win.c#n150 > I suppose dwProcessId might be guaranteed to be 0 for remote connections, but still. will add a result = 0; before the loop above, thanks for this pointer
(In reply to comment #10) > > @@ +229,5 @@ > > > + > > > + for (dwSize = 0; dwSize < lpBuffer->dwNumEntries; dwSize++) > > > + { > > > + int local_port = ntohs (lpBuffer->table[dwSize].dwLocalPort); > > > + if (local_port == peer_port) > > > > Isn't dwLocalPort the dbus-daemon end, and dwRemotePort the peer (other > > process) end? That's what I'd expect from the naming, anyway. > Previously working version uses dwLocalPort too > http://cgit.freedesktop.org/dbus/dbus/tree/dbus/dbus-sysdeps-win.c#n185. > Will double check this. The listed connection is owned by the client, so localPort is the port on the client side, which is the one we are searching for.
Created attachment 83832 [details] [review] Fixed bug of unsupported GetExtendedTcpTable() table option on Windows XP SP2 and earlier updated patch according to comment #10
(In reply to comment #11) > The listed connection is owned by the client, so localPort is the port on > the client side, which is the one we are searching for. Ah, I think I see... this API is basically Windows' equivalent of netstat(1) so it lists the pid owning the "local" end of each connection, and we're looking at the client process' attributes rather than our own, so from the perspective of this table, the dbus-daemon is "remote" and the client is "local". A bit confusing, but OK. > +/* > + * _MIB_TCPROW_EX and friends are not available in system headers > + * and are mapped to attribute identical ...OWNER_PID typedefs. > + */ > +typedef MIB_TCPROW_OWNER_PID _MIB_TCPROW_EX; > +typedef MIB_TCPTABLE_OWNER_PID MIB_TCPTABLE_EX; > +typedef PMIB_TCPTABLE_OWNER_PID PMIB_TCPTABLE_EX; If it's the same data structure behind the scenes (including types) then that's a much nicer way to express it. Thanks! > +static BOOL load_ex_ip_helper_procedures(void) I think this deserves a comment "AllocateAndGetTcpExTableFromStack() is undocumented and not exported, but is the only way to do this in older XP versions" or something similar. > > Don't you need to check that dwRemoteAddr is local, too? > This has been checked earlier in the function see http://cgit.freedesktop.org/dbus/dbus/tree/dbus/dbus-sysdeps-win.c#n150 That's not what I meant. You've checked that the peer is local, but you haven't checked that the row you found actually corresponds to the peer. Consider this (admittedly slightly contrived) situation: * we are listening on 127.0.0.1:54321 * the peer, pid 42, is connecting to us from 127.0.0.1:12345 * the TCP table looks like this: local remote pid 192.168.0.1:12345 8.8.8.8:80 23 127.0.0.1:12345 127.0.0.1:54321 42 (yes you can bind to the same port once per IP address) I think you'd be returning pid 23 in this situation, whereas you should be returning 42? This probably applies to the current code too - sorry I didn't spot that.
Created attachment 83843 [details] [review] Only take process id of localhost connections
Created attachment 83844 [details] [review] Add doc to load_ex_ip_helper_procedures()
Comment on attachment 83843 [details] [review] Only take process id of localhost connections Review of attachment 83843 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-sysdeps-win.c @@ +210,5 @@ > for (dwSize = 0; dwSize < lpBuffer->dwNumEntries; dwSize++) > { > int local_port = ntohs (lpBuffer->table[dwSize].dwLocalPort); > + int local_address = htonl (lpBuffer->table[dwSize].dwLocalAddr); > + if (local_address == tINADDR_LOOPBACK && local_port == peer_port) Is tINADDR_LOOPBACK (with the "t") something that really exists, or just a typo? This should be ntohl() not htonl(), because you're taking an address in network byte order (big-endian) and translating it into host byte order (usually little-endian). There is no technical difference, but using the wrong one confuses the reader :-) @@ +247,4 @@ > for (i = 0; i < tcp_table->dwNumEntries; i++) > { > MIB_TCPROW_OWNER_PID *p = &tcp_table->table[i]; > + int local_address = htonl (p->dwLocalAddr); ntohl() here too, please.
Comment on attachment 83844 [details] [review] Add doc to load_ex_ip_helper_procedures() Review of attachment 83844 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-sysdeps-win.c @@ +118,5 @@ > > +/** > + * AllocateAndGetTcpExTableFromStack() is undocumented and not exported, > + * but is the only way to do this in older XP versions. > + * @return true procedures could be loaded very very minor: "true if the procedures..."
(In reply to comment #16) > Comment on attachment 83843 [details] [review] [review] > Only take process id of localhost connections > > Review of attachment 83843 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: dbus/dbus-sysdeps-win.c > @@ +210,5 @@ > > for (dwSize = 0; dwSize < lpBuffer->dwNumEntries; dwSize++) > > { > > int local_port = ntohs (lpBuffer->table[dwSize].dwLocalPort); > > + int local_address = htonl (lpBuffer->table[dwSize].dwLocalAddr); > > + if (local_address == tINADDR_LOOPBACK && local_port == peer_port) > > Is tINADDR_LOOPBACK (with the "t") something that really exists, or just a > typo? typo while editing the commit message, sorry. > > This should be ntohl() not htonl(), because you're taking an address in > network byte order (big-endian) and translating it into host byte order > (usually little-endian). There is no technical difference, but using the > wrong one confuses the reader :-) > > @@ +247,4 @@ > > for (i = 0; i < tcp_table->dwNumEntries; i++) > > { > > MIB_TCPROW_OWNER_PID *p = &tcp_table->table[i]; > > + int local_address = htonl (p->dwLocalAddr); > > ntohl() here too, please. will do so. I took this from http://cgit.freedesktop.org/dbus/dbus/tree/dbus/dbus-sysdeps-win.c#n131, is this line also affected ?
Created attachment 83862 [details] [review] Only take process id of localhost connections (update)
Created attachment 83863 [details] [review] Add doc to load_ex_ip_helper_procedures() (update)
Created attachment 83864 [details] [review] Fix for broken wine implementation
Created attachment 83865 [details] [review] Refactored functions
Created attachment 83866 [details] [review] Add debug messages to load_ex_ip_helper_procedures()
(In reply to comment #18) > I took this from > http://cgit.freedesktop.org/dbus/dbus/tree/dbus/dbus-sysdeps-win.c#n131, is > this line also affected ? Yes it is: struct sockaddr_in is stored in network byte order, and you want to compare it with INADDR_LOOPBACK which is in host byte order. So that one should really be ntohl() too.
Comment on attachment 83862 [details] [review] Only take process id of localhost connections (update) Review of attachment 83862 [details] [review]: ----------------------------------------------------------------- ++
Comment on attachment 83863 [details] [review] Add doc to load_ex_ip_helper_procedures() (update) Review of attachment 83863 [details] [review]: ----------------------------------------------------------------- sure
Comment on attachment 83864 [details] [review] Fix for broken wine implementation Review of attachment 83864 [details] [review]: ----------------------------------------------------------------- So this is basically just changing the order: try the documented, >= XP SP3 method first, then if that fails and we're on <= XP SP2, go for the undocumented, <= XP SP2 version? Seems fair enough, and I definitely like the two mechanisms being factored out into helper functions. ::: dbus/dbus-sysdeps-win.c @@ +134,5 @@ > return TRUE; > } > > + > +dbus_pid_t get_pid_from_extended_tcp_table(int peer_port) This should be static. With a couple of coding style adjustments, that's: static dbus_pid_t get_pid_from_extended_tcp_table (int peer_port) @@ +150,5 @@ > + { > + _dbus_verbose ("Error allocating memory\n"); > + return 0; > + } > + } Not a new thing, but should there be an "else" clause on this? I realize GetExtendedTcpTable() has to fail like this when given a NULL first argument, but it seems as though either there shouldn't be an "if", or there should be an "else" that either warns and returns, or dies with an assertion failure. @@ +175,5 @@ > + dbus_free (tcp_table); > + return result; > +} > + > +dbus_pid_t get_pid_from_tcp_ex_table(int peer_port) Again, this should be static dbus_pid_t get_pid_from_tcp_ex_table (int peer_port) @@ +207,5 @@ > + result = lpBuffer->table[dwSize].dwOwningPid; > + break; > + } > + } > + if (lpBuffer) Not a new thing, but: if lpBuffer is NULL, you've already crashed out with a segfault when you dereferenced lpBuffer->dwNumEntries. Is it possible for AllocateAndGetTcpExTableFromStack to return NO_ERROR but not write a non-NULL value into lpBuffer? If it is, do an early-return for that; if it isn't, this check is redundant. @@ +267,4 @@ > return 0; > } > > + _dbus_verbose ("\n"); Why this \n?
Created attachment 83891 [details] [review] Fixed remaining issues
Comment on attachment 83891 [details] [review] Fixed remaining issues Review of attachment 83891 [details] [review]: ----------------------------------------------------------------- thanks, ++
(In reply to comment #29) > Comment on attachment 83891 [details] [review] [review] > Fixed remaining issues > > Review of attachment 83891 [details] [review] [review]: > ----------------------------------------------------------------- > > thanks, ++ good, will commmit them to master.
committed to master, thanks for reviewing
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.