Bug 89444 - should not assume that sockets are of type int
Summary: should not assume that sockets are of type int
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: git master
Hardware: Other All
: medium normal
Assignee: D-Bus Maintainers
QA Contact: D-Bus Maintainers
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 90314
  Show dependency treegraph
 
Reported: 2015-03-05 12:27 UTC by Simon McVittie
Modified: 2015-05-12 19:06 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
DBusSocket refactoring - unix part (2.26 KB, patch)
2015-03-05 18:24 UTC, Ralf Habacker
Details | Splinter Review
DBusSocket refactoring - windows part (8.47 KB, patch)
2015-03-05 18:24 UTC, Ralf Habacker
Details | Splinter Review
DBusSocket refactoring - common part (11.44 KB, patch)
2015-03-05 18:24 UTC, Ralf Habacker
Details | Splinter Review
DBusSocket refactoring - windows part (update) (6.27 KB, patch)
2015-03-05 18:27 UTC, Ralf Habacker
Details | Splinter Review
Refactor references to socket fd's to use DBusSocket to avoid conversion warnings (19.34 KB, patch)
2015-03-05 18:52 UTC, Ralf Habacker
Details | Splinter Review
Use typedef DBusSocket for sockets fd's to avoid conversion warnings (24.72 KB, patch)
2015-03-05 20:17 UTC, Ralf Habacker
Details | Splinter Review
Revert partially DBusSocket refactoring, it is not appliable (to be merged into the main patch) (1.26 KB, patch)
2015-03-05 20:32 UTC, Ralf Habacker
Details | Splinter Review
Rename _dbus_full_duplex_pipe() to more descriptive name _dbus_socketpair() (4.17 KB, patch)
2015-03-06 07:11 UTC, Ralf Habacker
Details | Splinter Review
Complete rename to _dbus_socketpair() (this patch is going to be merged into the related base patch) (3.62 KB, patch)
2015-03-06 07:18 UTC, Ralf Habacker
Details | Splinter Review
Rename _dbus_full_duplex_pipe() to more descriptive name _dbus_socketpair(). (7.28 KB, patch)
2015-03-06 18:27 UTC, Ralf Habacker
Details | Splinter Review
Use typedef DBusSocket for sockets fd's to avoid conversion warnings (24.34 KB, patch)
2015-03-06 18:41 UTC, Ralf Habacker
Details | Splinter Review
Use typedef DBusSocket for sockets fd's to avoid conversion warnings (update) (22.83 KB, patch)
2015-03-08 11:06 UTC, Ralf Habacker
Details | Splinter Review
[01/11] Use typedef DBusSocket for sockets fd's to avoid conversion warnings (24.38 KB, patch)
2015-03-31 12:13 UTC, Simon McVittie
Details | Splinter Review
[02/11] bus_unix_fds_passing_test: the results of _dbus_socketpair are sockets (1.76 KB, patch)
2015-03-31 12:13 UTC, Simon McVittie
Details | Splinter Review
[03/11] dbus-sysdeps: add more infrastructure around DBusSocket (3.11 KB, patch)
2015-03-31 12:13 UTC, Simon McVittie
Details | Splinter Review
[04/11] Mostly remove the remnants of an older socket abstraction layer (1.83 KB, patch)
2015-03-31 12:14 UTC, Simon McVittie
Details | Splinter Review
[05/11] DBusMainLoop, DBusSocketSet: work in terms of DBusPollable (24.71 KB, patch)
2015-03-31 12:15 UTC, Simon McVittie
Details | Splinter Review
[06/11] main: reload_pipe is (despite its name) a socket pair (1.12 KB, patch)
2015-03-31 12:16 UTC, Simon McVittie
Details | Splinter Review
[07/11] Split _dbus_set_fd_nonblocking vs. _dbus_set_socket_nonblocking (4.55 KB, patch)
2015-03-31 12:16 UTC, Simon McVittie
Details | Splinter Review
[08/11] generic socket transport code: work in terms of DBusSocket (10.65 KB, patch)
2015-03-31 12:16 UTC, Simon McVittie
Details | Splinter Review
[09/11] Convert miscellaneous socket APIs to DBusSocket (14.91 KB, patch)
2015-03-31 12:17 UTC, Simon McVittie
Details | Splinter Review
[10/11] Remove _dbus_socket_is_invalid, no longer used (1.88 KB, patch)
2015-03-31 12:17 UTC, Simon McVittie
Details | Splinter Review
[11/11] Turn DBusSocket into a type-safe struct, preventing inappropriate conversion (46.75 KB, patch)
2015-03-31 12:20 UTC, Simon McVittie
Details | Splinter Review
[01/11] Use typedef DBusSocket for sockets fd's to avoid conversion warnings (rebased) (23.00 KB, patch)
2015-03-31 19:55 UTC, Ralf Habacker
Details | Splinter Review
Convert mostly DBUS_SOCKET_... and DBUS_POLLABLE_.. macros to inline functions for more type safety. (21.51 KB, patch)
2015-04-16 21:18 UTC, Ralf Habacker
Details | Splinter Review
[fixup for 07/11] remove duplicate declaration of _dbus_set_socket_nonblocking (1008 bytes, patch)
2015-04-17 13:13 UTC, Simon McVittie
Details | Splinter Review
[fixup for Attachment #115135] style: space before parenthesis in declarations and definitions (2.28 KB, patch)
2015-04-17 13:14 UTC, Simon McVittie
Details | Splinter Review
[13] DBusSocket: put the #ifdef case before the !defined case (2.48 KB, patch)
2015-04-17 13:17 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 2015-03-05 12:27:08 UTC
On Unix, sockets are just file descriptors, which are of type int. -1 is the invalid non-socket value used to represent errors.

On Windows, sockets are correctly of type SOCKET, which is a UINT_PTR (an unsigned type of size equal to pointer, like ISO C uintptr_t), and INVALID_SOCKET is the invalid non-socket value used to represent errors.

At the moment, libdbus "sysdeps" methods that take a socket fd take an int. This is clearly wrong, and leads to warnings like this:

/home/ralf.habacker/src/dbus-master/dbus/dbus-sysdeps-win.c: In function '_dbus_socket_is_invalid':
/home/ralf.habacker/src/dbus-master/dbus/dbus-sysdeps-win.c:649:15: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
     return fd == INVALID_SOCKET ? TRUE : FALSE;

We should either do this in dbus-sysdeps.h instead:

#ifdef DBUS_WIN
typedef int DBusSocket;
#else
typedef SOCKET DBusSocket;
#endif

and make _dbus_close_socket() and similar functions take a DBusSocket instead of an int; either that, or use a type-safe struct

typedef struct {
#ifdef DBUS_WIN
  SOCKET windows_socket;
#else
  int unix_fd;
#endif
} DBusSocket;

and pass/return it by value, if that's something we can do portably.

We cannot do this for public APIs like dbus_connection_get_socket(), which must continue to have their current types (with a cast from the internal representation if necessary).

Unix-specific functions can continue to take and return int: the fact that fds are ints, and the fact that sockets are just fds, is quite a fundamental Unix ABI thing. Similarly, Windows-specific functions can use SOCKET or HANDLE directly if desired.
Comment 1 Ralf Habacker 2015-03-05 18:23:39 UTC
(In reply to Simon McVittie from comment #0)

> #ifdef DBUS_WIN
> typedef int DBusSocket;
> #else
> typedef SOCKET DBusSocket;
> #endif

This one is less invasive. 

> 
> typedef struct {
> #ifdef DBUS_WIN
>   SOCKET windows_socket;
> #else
>   int unix_fd;
> #endif
> } DBusSocket;
> 

this requires to refactor any access to socket fd's and is much more work. I would prefer the first one.
  

> Unix-specific functions can continue to take and return int: the fact that
> fds are ints, and the fact that sockets are just fds, is quite a fundamental
> Unix ABI thing. Similarly, Windows-specific functions can use SOCKET or
> HANDLE directly if desired.

Sure, I'm not sure, when to use the first and when the other. With the appended patch I tried to change this.
Comment 2 Ralf Habacker 2015-03-05 18:24:07 UTC
Created attachment 114033 [details] [review]
DBusSocket refactoring - unix part
Comment 3 Ralf Habacker 2015-03-05 18:24:24 UTC
Created attachment 114034 [details] [review]
DBusSocket refactoring - windows part
Comment 4 Ralf Habacker 2015-03-05 18:24:45 UTC
Created attachment 114035 [details] [review]
DBusSocket refactoring - common part
Comment 5 Ralf Habacker 2015-03-05 18:27:40 UTC
Created attachment 114036 [details] [review]
DBusSocket refactoring - windows part (update)

remved some unrelated changes
Comment 6 Simon McVittie 2015-03-05 18:36:43 UTC
Comment on attachment 114034 [details] [review]
DBusSocket refactoring - windows part

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

This can't be applied until after the common part is, afaics.

I'd prefer all the s/int/DBusSocket/ to be one commit really, without any of the unrelated changes that seem to have crept in - I don't think there's any need to split it into Windows/Unix/common.

::: dbus/dbus-spawn-win.c
@@ +77,5 @@
>      char **envp;
>  
>      HANDLE child_handle;
> +    DBusSocket socket_to_babysitter;	/* Connection to the babysitter thread */
> +    DBusSocket socket_to_main;

These are really sockets?

::: dbus/dbus-sysdeps-util-win.c
@@ -380,4 @@
>    const char *filename_c;
>    WIN32_FILE_ATTRIBUTE_DATA wfad;
>    char *lastdot;
> -  DWORD rc;

Not actually part of this change afaics?

::: dbus/dbus-sysdeps-win.c
@@ -52,4 @@
>  #include "dbus-credentials.h"
>  
>  #include <windows.h>
> -#include <ws2tcpip.h>

Why?

@@ +645,2 @@
>  dbus_bool_t
>  _dbus_socket_is_invalid (int fd)

This function should take a DBusSocket too, then you wouldn't need...

@@ +645,4 @@
>  dbus_bool_t
>  _dbus_socket_is_invalid (int fd)
>  {
> +    return fd == (int)INVALID_SOCKET ? TRUE : FALSE;

... this cast.

@@ +1054,5 @@
>   * @returns #FALSE on failure (if error is set)
>   */
>  dbus_bool_t
> +_dbus_full_duplex_pipe (DBusSocket *fd1,
> +                        DBusSocket *fd2,

Not your fault, and not a merge blocker for this change, but the name of this function is silly (a pipe is a jargon word, and this is not a pipe), and making this change to it makes that even more obvious.

Perhaps we should call it _dbus_socketpair() since it has exactly the semantics "do the same as socketpair() on Unix, or as close to that as you can achieve on Windows"...

@@ +1706,5 @@
>    tmp = ai;
>    while (tmp)
>      {
> +      DBusSocket fd = INVALID_SOCKET, *newlisten_fd;
> +      if ((fd = (DBusSocket)socket (tmp->ai_family, SOCK_STREAM, 0)) == INVALID_SOCKET)

Why the cast? MSDN says socket() returns a SOCKET already, and you're changing DBusSocket to be exactly a SOCKET?

@@ +2271,4 @@
>    if (!_dbus_string_lengthen (str, n_bytes))
>      return FALSE;
>  
> +  p = (BYTE*)_dbus_string_get_data_len (str, old_len, n_bytes);

This seems to be nothing to do with sockets?

It does make sense, assuming the Windows crypto API you're using is done in terms of BYTE*, but it's really part of Bug #15522. I still think the right answer to that bug is to add _dbus_string_get_udata[_len] static inline functions that encapsulate the cast.

@@ +2307,4 @@
>      {
>        char *last_slash;
>  
> +      if (!GetTempPathA (sizeof (buf), (LPSTR)buf))

Why the cast?

Is this really a Bug #15522 change?

@@ +2313,5 @@
>            _dbus_abort ();
>          }
>  
>        /* Drop terminating backslash or slash */
> +      last_slash = strrchr (buf, '\\');

This change is nothing to do with sockets.

@@ +2318,3 @@
>        if (last_slash > buf && last_slash[1] == '\0')
>          last_slash[0] = '\0';
> +      last_slash = strrchr (buf, '/');

Neither's this.

@@ +3292,4 @@
>      DWORD pathLength;
>      char *lastSlash;
>      SetLastError( 0 );
> +    pathLength = GetModuleFileNameA(_dbus_win_get_dll_hmodule(), (LPSTR)prefix, len);

Neither's this.

@@ +3296,5 @@
>      if ( pathLength == 0 || GetLastError() != 0 ) {
>          *prefix = '\0';
>          return FALSE;
>      }
> +    lastSlash = strrchr(prefix, '\\');

Or this.
Comment 7 Simon McVittie 2015-03-05 18:37:27 UTC
Sorry, I see you've already fixed some of my review comments; please disregard where appropriate.
Comment 8 Ralf Habacker 2015-03-05 18:52:53 UTC
Created attachment 114037 [details] [review]
Refactor references to socket fd's to use DBusSocket to avoid conversion warnings
Comment 9 Simon McVittie 2015-03-05 18:55:30 UTC
Comment on attachment 114035 [details] [review]
DBusSocket refactoring - common part

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

::: dbus/dbus-server-socket.c
@@ +51,4 @@
>  {
>    DBusServer base;   /**< Parent class members. */
>    int n_fds;         /**< Number of active file handles */
> +  DBusSocket *fds;   /**< File descriptor or -1 if disconnected. */

This suggests that -1 is used elsewhere in this file, and, sure enough, it is. The use of -1 should be some platform-dependent invalid value.

I think that means we need

#ifdef DBUS_WIN
typedef SOCKET DBusSocket;
# define DBUS_SOCKET_INVALID INVALID_SOCKET
#else
typedef int DBusSocket;
# define DBUS_SOCKET_INVALID -1
#endif

or something similar.

@@ +187,3 @@
>        int saved_errno;
>  
>        listen_fd = dbus_watch_get_socket (watch);

Don't we now need to introduce DBusSocket _dbus_watch_get_socket (DBusWatch *) so that it can return a SOCKET on Windows, without truncation on 64-bit?

(I want to keep DBusSocket out of the public API at least for now - it's hard to get right without breaking our policy of "public headers don't include system headers". I'll have to think about the best way to implement a SOCKET-based public API without mentioning SOCKET. For now, just keep it private.)

@@ +295,4 @@
>  
>    socket_server->noncefile = noncefile;
>  
> +  socket_server->fds = (DBusSocket *)dbus_new (int, n_fds);

The need for a cast should be a clue that this is wrong.

On Win64 this is a buffer overflow, because int is 4 bytes and SOCKET = uintptr_t is 8 bytes, so you're giving fds less space than it needs.

dbus_new (DBusSocket, n_fds) would be fine.

::: dbus/dbus-sysdeps.h
@@ +132,5 @@
> +typedef int DBusSocket;
> +#else
> +#include <ws2tcpip.h>
> +typedef SOCKET DBusSocket;
> +#endif

Hmm, the need to add that #include isn't ideal - I thought we had it already - but this is a private header so I suppose it's OK.

I'd prefer the #include done further up the file with the others.

@@ +159,3 @@
>                                            DBusString       *buffer,
>                                            int               count,
> +                                          DBusSocket       *fds,

This array should still be an array of ints, not DBusSockets. Unix fd-passing can pass any sort of fd - file, pipe, socket, whatever - so these are not conceptually sockets.

@@ +384,4 @@
>                                                      DBusError *error);
>  
>  DBUS_PRIVATE_EXPORT
> +const char *_dbus_get_tmpdir(void);

Why this change?

Our coding style in headers is to either line up   (arguments
all the way                                         over here)
so that                                            (multiple functions)
line up

(I personally dislike that convention, but it's the prevailing style in dbus) or to have one space before the opening parenthesis.

::: dbus/dbus-transport-socket.c
@@ +604,4 @@
>            if (socket_transport->message_bytes_written <= 0 && DBUS_TRANSPORT_CAN_SEND_UNIX_FD(transport))
>              {
>                /* Send the fds along with the first byte of the message */
> +              const DBusSocket *unix_fds;

As before, these are not conceptually sockets (even if some of the fds happen to point to a socket really).

@@ +821,5 @@
>  #ifdef HAVE_UNIX_FD_PASSING
>        if (DBUS_TRANSPORT_CAN_SEND_UNIX_FD(transport))
>          {
> +          DBusSocket *fds;
> +          int n_fds;

And here.
Comment 10 Simon McVittie 2015-03-05 18:57:12 UTC
(In reply to Ralf Habacker from comment #2)
> DBusSocket refactoring - unix part

No comments that I haven't already flagged in the common or Windows part.
Comment 11 Simon McVittie 2015-03-05 19:02:27 UTC
(In reply to Ralf Habacker from comment #1)
> this requires to refactor any access to socket fd's and is much more work. I
> would prefer the first one.

Yeah, that's a valid concern.

Doing the first one now does not rule out doing the second later, and is in fact a step towards it anyway.
Comment 12 Ralf Habacker 2015-03-05 19:08:30 UTC
(In reply to Simon McVittie from comment #6)
> Comment on attachment 114034 [details] [review] [review]
> DBusSocket refactoring - windows part
> 
> Review of attachment 114034 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> This can't be applied until after the common part is, afaics.
> 
> I'd prefer all the s/int/DBusSocket/ to be one commit really, without any of
> the unrelated changes that seem to have crept in - I don't think there's any
> need to split it into Windows/Unix/common.
> 
> ::: dbus/dbus-spawn-win.c
> @@ +77,5 @@
> >      char **envp;
> >  
> >      HANDLE child_handle;
> > +    DBusSocket socket_to_babysitter;	/* Connection to the babysitter thread */
> > +    DBusSocket socket_to_main;
> 
> These are really sockets?

they are used in a call to _dbus_close_socket()

> ::: dbus/dbus-sysdeps-win.c
> @@ -52,4 @@
> >  #include "dbus-credentials.h"
> >  
> >  #include <windows.h>
> > -#include <ws2tcpip.h>
> 
> Why?
the definitions has been moved to dbus-sysdeps.h to define SOCKET

> 
> @@ +645,2 @@
> >  dbus_bool_t
> >  _dbus_socket_is_invalid (int fd)
> 
> This function should take a DBusSocket too, then you wouldn't need...
sure, I will add a related patch 

> 
> @@ +1054,5 @@
> >   * @returns #FALSE on failure (if error is set)
> >   */
> >  dbus_bool_t
> > +_dbus_full_duplex_pipe (DBusSocket *fd1,
> > +                        DBusSocket *fd2,
> 
> Not your fault, and not a merge blocker for this change, but the name of
> this function is silly (a pipe is a jargon word, and this is not a pipe),
> and making this change to it makes that even more obvious.
> 
> Perhaps we should call it _dbus_socketpair() since it has exactly the
> semantics "do the same as socketpair() on Unix, or as close to that as you
> can achieve on Windows"...
I will take a look

> 
> @@ +1706,5 @@
> >    tmp = ai;
> >    while (tmp)
> >      {
> > +      DBusSocket fd = INVALID_SOCKET, *newlisten_fd;
> > +      if ((fd = (DBusSocket)socket (tmp->ai_family, SOCK_STREAM, 0)) == INVALID_SOCKET)
> 
> Why the cast? MSDN says socket() returns a SOCKET already, and you're
> changing DBusSocket to be exactly a SOCKET?

I will take a look 

> @@ +2271,4 @@
> >    if (!_dbus_string_lengthen (str, n_bytes))
> >      return FALSE;
> >  
> > +  p = (BYTE*)_dbus_string_get_data_len (str, old_len, n_bytes);
> 
> This seems to be nothing to do with sockets?
yes, sorry
Comment 13 Ralf Habacker 2015-03-05 20:06:32 UTC
Got a warning: 
/home/ralf/src/dbus-2/dbus/dbus-sysdeps-win.c: In function '_dbus_write_socket_two':
/home/ralf/src/dbus-2/dbus/dbus-sysdeps-win.c:639:21: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
   if (bytes_written < 0 && errno == EINTR)
                     ^
Comment 14 Ralf Habacker 2015-03-05 20:09:02 UTC
(In reply to Simon McVittie from comment #9)
> @@ +187,3 @@
> >        int saved_errno;
> >  
> >        listen_fd = dbus_watch_get_socket (watch);
> 
> Don't we now need to introduce DBusSocket _dbus_watch_get_socket (DBusWatch
> *) so that it can return a SOCKET on Windows, without truncation on 64-bit?
> 
> (I want to keep DBusSocket out of the public API at least for now - it's
> hard to get right without breaking our policy of "public headers don't
> include system headers". I'll have to think about the best way to implement
> a SOCKET-based public API without mentioning SOCKET. For now, just keep it
> private.)
> 
Adding a private function ?
DBUS_PRIVATE_EXPORT
DBusSocket     _dbus_watch_get_socket         (DBusWatch               *watch);
Comment 15 Ralf Habacker 2015-03-05 20:17:45 UTC
Created attachment 114041 [details] [review]
Use typedef DBusSocket for sockets fd's to avoid conversion warnings
Comment 16 Ralf Habacker 2015-03-05 20:30:10 UTC
(In reply to Simon McVittie from comment #9)
> Comment on attachment 114035 [details] [review] [review]
> DBusSocket refactoring - common part
> 
> Review of attachment 114035 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: dbus/dbus-server-socket.c
> @@ +51,4 @@
> >  {
> >    DBusServer base;   /**< Parent class members. */
> >    int n_fds;         /**< Number of active file handles */
> > +  DBusSocket *fds;   /**< File descriptor or -1 if disconnected. */
> 
> This suggests that -1 is used elsewhere in this file, and, sure enough, it
> is. The use of -1 should be some platform-dependent invalid value.
> 
> I think that means we need
> 
> #ifdef DBUS_WIN
> typedef SOCKET DBusSocket;
> # define DBUS_SOCKET_INVALID INVALID_SOCKET
> #else
> typedef int DBusSocket;
> # define DBUS_SOCKET_INVALID -1
> #endif
> 
> or something similar.

fixed with attachment 114041 [details] [review]
 
> @@ +187,3 @@
> >        int saved_errno;
> >  
> >        listen_fd = dbus_watch_get_socket (watch);
> 
> Don't we now need to introduce DBusSocket _dbus_watch_get_socket (DBusWatch
> *) so that it can return a SOCKET on Windows, without truncation on 64-bit?
> 
> (I want to keep DBusSocket out of the public API at least for now - it's
> hard to get right without breaking our policy of "public headers don't
> include system headers". I'll have to think about the best way to implement
> a SOCKET-based public API without mentioning SOCKET. For now, just keep it
> private.)

fixed with attachment 114041 [details] [review]
 
> @@ +295,4 @@
> >  
> >    socket_server->noncefile = noncefile;
> >  
> > +  socket_server->fds = (DBusSocket *)dbus_new (int, n_fds);
> 
> The need for a cast should be a clue that this is wrong.
> 
> On Win64 this is a buffer overflow, because int is 4 bytes and SOCKET =
> uintptr_t is 8 bytes, so you're giving fds less space than it needs.
> 
> dbus_new (DBusSocket, n_fds) would be fine.

fixed with attachment 114041 [details] [review]

> ::: dbus/dbus-sysdeps.h
> @@ +132,5 @@
> > +typedef int DBusSocket;
> > +#else
> > +#include <ws2tcpip.h>
> > +typedef SOCKET DBusSocket;
> > +#endif
> 
> Hmm, the need to add that #include isn't ideal - I thought we had it already
> - but this is a private header so I suppose it's OK.
> 
> I'd prefer the #include done further up the file with the others.

fixed with attachment 114041 [details] [review]

> @@ +159,3 @@
> >                                            DBusString       *buffer,
> >                                            int               count,
> > +                                          DBusSocket       *fds,
> 
> This array should still be an array of ints, not DBusSockets. Unix
> fd-passing can pass any sort of fd - file, pipe, socket, whatever - so these
> are not conceptually sockets.

fixed with attachment 114041 [details] [review]

> ::: dbus/dbus-transport-socket.c
> @@ +604,4 @@
> >            if (socket_transport->message_bytes_written <= 0 && DBUS_TRANSPORT_CAN_SEND_UNIX_FD(transport))
> >              {
> >                /* Send the fds along with the first byte of the message */
> > +              const DBusSocket *unix_fds;
> 
> As before, these are not conceptually sockets (even if some of the fds
> happen to point to a socket really).
> 
still open

> @@ +821,5 @@
> >  #ifdef HAVE_UNIX_FD_PASSING
> >        if (DBUS_TRANSPORT_CAN_SEND_UNIX_FD(transport))
> >          {
> > +          DBusSocket *fds;
> > +          int n_fds;
> 
> And here.

still open
Comment 17 Ralf Habacker 2015-03-05 20:32:43 UTC
Created attachment 114042 [details] [review]
Revert partially DBusSocket refactoring, it is not appliable (to be merged into the main patch)
Comment 18 Ralf Habacker 2015-03-06 07:11:18 UTC
Created attachment 114069 [details] [review]
Rename _dbus_full_duplex_pipe() to more descriptive name _dbus_socketpair()
Comment 19 Ralf Habacker 2015-03-06 07:18:18 UTC
Created attachment 114070 [details] [review]
Complete rename to _dbus_socketpair() (this patch is going to be merged into the related base patch)
Comment 20 Simon McVittie 2015-03-06 11:57:57 UTC
I'll review these soon, but it'll take a little while to check that all the uses of -1 vs. INVALID_SOCKET are appropriate, so it'll have to wait til I have time to go through it all.

If you want to squash the patches into fewer before I review, so that the patches are in the intended order and each one is individually correct, that would be great; or if you don't, I'll do that myself before reviewing, to avoid complaining about things that you actually fixed in a later patch :-)
Comment 21 Ralf Habacker 2015-03-06 18:27:41 UTC
Created attachment 114092 [details] [review]
Rename _dbus_full_duplex_pipe() to more descriptive name _dbus_socketpair().
Comment 22 Ralf Habacker 2015-03-06 18:41:50 UTC
Created attachment 114093 [details] [review]
Use typedef DBusSocket for sockets fd's to avoid conversion warnings

I splitted the reviewed stuff into 

1. simple rename of _dbus_full_duplex_pipe() to _dbus_socketpair ()
2. Introduce typedef DBusSocket.
Comment 23 Simon McVittie 2015-03-06 19:12:23 UTC
Thanks, I'll come back to this next week.
Comment 24 Ralf Habacker 2015-03-08 11:06:02 UTC
Created attachment 114132 [details] [review]
Use typedef DBusSocket for sockets fd's to avoid conversion warnings (update)

- removed unrelated rename to DBusSocket (has been already rejected)
Comment 25 Simon McVittie 2015-03-10 13:16:47 UTC
Comment on attachment 114132 [details] [review]
Use typedef DBusSocket for sockets fd's to avoid conversion warnings (update)

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

::: dbus/dbus-connection.h
@@ +31,4 @@
>  #include <dbus/dbus-memory.h>
>  #include <dbus/dbus-message.h>
>  #include <dbus/dbus-shared.h>
> +#include <dbus/dbus-sysdeps.h>

This is not valid. Public, installed headers like dbus-connection.h cannot include internal, non-installed headers like dbus-sysdeps.h.

Removing this line seems to be OK as a fix - nothing seems to be relying on adding this. Perhaps it was relied on in an earlier version of your patches.

::: dbus/dbus-mainloop.c
@@ +333,2 @@
>  
>    watches = _dbus_hash_table_lookup_int (loop->watches, fd);

This is implicitly converting from SOCKET to (signed) int. There's lots of other code in this file doing the same, unfortunately... this patch is a step in the right direction, but we would still corrupt any SOCKET whose value is not a fairly small positive integer.

::: dbus/dbus-sysdeps-win.c
@@ +1508,4 @@
>                                       const char     *noncefile,
>                                       DBusError      *error)
>  {
> +  SOCKET fd = DBUS_SOCKET_INVALID, res;

res isn't a SOCKET - getattr still returns int.

::: dbus/dbus-watch.c
@@ +592,5 @@
>  
> +DBusSocket
> +_dbus_watch_get_socket (DBusWatch *watch)
> +{
> +  _dbus_return_val_if_fail (watch != NULL, -1);

Calling _dbus_return_val_if_fail() from a function whose name starts with "_" triggers an assertion failure; for internal functions you're meant to use _dbus_assert() instead.

(Did you test this? Running the tests finds this immediately.)
Comment 26 Simon McVittie 2015-03-10 13:19:39 UTC
(In reply to Ralf Habacker from comment #24)
> Use typedef DBusSocket for sockets fd's to avoid conversion warnings (update)

This also forgets to change one of the versions of _dbus_socketpair() to take DBusSocket * parameters.
Comment 27 Simon McVittie 2015-03-10 15:11:48 UTC
(In reply to Simon McVittie from comment #20)
> I'll review these soon, but it'll take a little while to check that all the
> uses of -1 vs. INVALID_SOCKET are appropriate, so it'll have to wait til I
> have time to go through it all.

The short version is, they're not.

I don't think we're going to get dbus to have actually correct socket handling unless/until we do this:

(In reply to Simon McVittie from comment #0)
> typedef struct {
> #ifdef DBUS_WIN
>   SOCKET windows_socket;
> #else
>   int unix_fd;
> #endif
> } DBusSocket;
> 
> and pass/return it by value

My work-in-progress patch for that is +480 -344 lines, which is not actually as bad as I had feared.
Comment 28 Ralf Habacker 2015-03-10 16:24:52 UTC
(In reply to Simon McVittie from comment #25)
> Comment on attachment 114132 [details] [review] [review]
> Use typedef DBusSocket for sockets fd's to avoid conversion warnings (update)
> 
> Review of attachment 114132 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> > +#include <dbus/dbus-sysdeps.h>

> Removing this line seems to be OK as a fix - nothing seems to be relying on
> adding this. Perhaps it was relied on in an earlier version of your patches.

yes I can confirmn 
 
> res isn't a SOCKET - getattr still returns int.

sure

> 
> ::: dbus/dbus-watch.c
> @@ +592,5 @@
> >  
> > +DBusSocket
> > +_dbus_watch_get_socket (DBusWatch *watch)
> > +{
> > +  _dbus_return_val_if_fail (watch != NULL, -1);
> 
> Calling _dbus_return_val_if_fail() from a function whose name starts with
> "_" triggers an assertion failure; for internal functions you're meant to
> use _dbus_assert() instead.
> 
> (Did you test this? Running the tests finds this immediately.)

I compiled dbus with the patches from this bug with autotools and did run make check

make[3]: Entering directory `/home/ralf.habacker/src/dbus-master-autotools-build/test'
make[4]: Entering directory `/home/ralf.habacker/src/dbus-master-autotools-build/test'
PASS: test-shell
PASS: test-printf
PASS: test-corrupt
PASS: test-dbus-daemon
PASS: test-dbus-daemon-eavesdrop
PASS: test-fdpass
PASS: test-monitor
PASS: test-loopback
PASS: test-marshal
PASS: test-refs
PASS: test-relay
PASS: test-sd-activation
PASS: test-syntax
PASS: test-syslog
PASS: test-uid-permissions
============================================================================
Testsuite summary for dbus 1.9.15
============================================================================
# TOTAL: 15
# PASS:  15
Comment 29 Simon McVittie 2015-03-12 16:55:44 UTC
(In reply to Ralf Habacker from comment #28)
> I compiled dbus with the patches from this bug with autotools and did run
> make check

Ah, I see what has happened. You didn't configure with --enable-embedded-tests (or --enable-developer), so you only got a subset of the tests, which happens to not have any calls to that function.
Comment 30 Simon McVittie 2015-03-12 17:00:26 UTC
(In reply to Simon McVittie from comment #27)
> I don't think we're going to get dbus to have actually correct socket
> handling unless/until we do this:
> 
> (In reply to Simon McVittie from comment #0)
> > typedef struct {
> > #ifdef DBUS_WIN
> >   SOCKET windows_socket;
> > #else
> >   int unix_fd;
> > #endif
> > } DBusSocket;
> > 
> > and pass/return it by value
> 
> My work-in-progress patch for that is +480 -344 lines, which is not actually
> as bad as I had feared.

It's a bit more complicated than that because we need a representation for "thing we can poll", which on Unix means any fd, but on Windows specifically means a SOCKET; and on Unix, for tedious reasons involving abuse of struct pollfd, we need the representation of "thing we can poll" to be exactly "int".

I have a mostly working branch with three layers of type:

DBusSocket: struct containing either SOCKET or int
    (a struct so that the compiler won't let us assign to/from int)
    - used in non-OS-specific socket APIs and high(ish)-level OS-specifics

DBusPollable: on Windows, DBusSocket (not SOCKET so that the compiler will
    stop us doing anything incorrect); on Unix, int
    - used in DBusWatch, DBusSocketSet, DBusLoop

Native type: SOCKET or int as appropriate
    - used in low-level OS-specific socket APIs
Comment 31 Simon McVittie 2015-03-12 20:39:13 UTC
Work in progress, might not work perfectly yet:

http://cgit.freedesktop.org/~smcv/dbus/log/?h=wip-dbussocket
Comment 32 Ralf Habacker 2015-03-17 16:05:53 UTC
(In reply to Simon McVittie from comment #31)
> Work in progress, might not work perfectly yet:
> 
> http://cgit.freedesktop.org/~smcv/dbus/log/?h=wip-dbussocket

I did run cmake on linux

cmake -DDBUS_ENABLE_EMBEDDED_TESTS=1 ../dbus-smcv/cmake/ 

and got

x@y:~/src/dbus-smcv-build> make
[  0%] Building C object dbus/CMakeFiles/dbus-1.dir/home/xxx/src/dbus-smcv/dbus/dbus-sysdeps-unix.o
/home/xxx/src/dbus-smcv/dbus/dbus-sysdeps-unix.c: In function ‘_dbus_write_socket_two’:
/home/xxx/src/dbus-smcv/dbus/dbus-sysdeps-unix.c:605:27: error: incompatible type for argument 1 of ‘_dbus_write_two’
                           buffer2, start2, len2);
                           ^
In file included from /home/xxx/src/dbus-smcv/dbus/dbus-sysdeps-unix.c:29:0:
/home/xxx/src/dbus-smcv/dbus/dbus-sysdeps-unix.h:61:1: note: expected ‘int’ but argument is of type ‘DBusSocket’
 _dbus_write_two (int               fd,
 ^
/home/xxx/src/dbus-smcv/dbus/dbus-sysdeps-unix.c: In function ‘add_linux_security_label_to_credentials’:
/home/xxx/src/dbus-smcv/dbus/dbus-sysdeps-unix.c:1693:30: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
       if (e != ERANGE || len <= _dbus_string_get_length_uint (&buf))
                              ^
/home/xxx/src/dbus-smcv/dbus/dbus-sysdeps-unix.c:1718:11: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
   if (len > _dbus_string_get_length_uint (&buf))
           ^
/home/xxx/src/dbus-smcv/dbus/dbus-sysdeps-unix.c:1740:51: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
   if (strlen (_dbus_string_get_const_data (&buf)) != len)
                                                   ^
make[2]: *** [dbus/CMakeFiles/dbus-1.dir/home/xxx/src/dbus-smcv/dbus/dbus-sysdeps-unix.o] Fehler 1
make[1]: *** [dbus/CMakeFiles/dbus-1.dir/all] Fehler 2
Comment 33 Simon McVittie 2015-03-17 19:37:38 UTC
(In reply to Ralf Habacker from comment #32)
> x@y:~/src/dbus-smcv-build> make
> [  0%] Building C object
> dbus/CMakeFiles/dbus-1.dir/home/xxx/src/dbus-smcv/dbus/dbus-sysdeps-unix.o
> /home/xxx/src/dbus-smcv/dbus/dbus-sysdeps-unix.c: In function
> ‘_dbus_write_socket_two’:
> /home/xxx/src/dbus-smcv/dbus/dbus-sysdeps-unix.c:605:27: error: incompatible
> type for argument 1 of ‘_dbus_write_two’
>                            buffer2, start2, len2);

Hmm. Did you apply the whole branch? I could build it using cmake 3.0 on Debian, targeting either Linux (native gcc) or Windows (mingw), without these errors.

I got some compiler warnings on mingw at intermediate points along the branch, because the fundamental issue here is that we're doing various implied conversions between SOCKET and int, and gcc will warn if they are like this:

    if (my_SOCKET == my_int)

but not if they're like this:

    my_function_that_takes_an_int (my_SOCKET)

and in any case, those warnings go away after the whole branch has been applied.

> /home/xxx/src/dbus-smcv/dbus/dbus-sysdeps-unix.c: In function
> ‘add_linux_security_label_to_credentials’:
> /home/xxx/src/dbus-smcv/dbus/dbus-sysdeps-unix.c:1693:30: warning:
> comparison between signed and unsigned integer expressions [-Wsign-compare]
>        if (e != ERANGE || len <= _dbus_string_get_length_uint (&buf))
>                               ^

I think this indicates that your build thinks socklen_t is signed? (It isn't - or it's uint32 on x86-64 Linux, at least. See <bits/types.h>.)
Comment 34 Simon McVittie 2015-03-17 19:50:43 UTC
(In reply to Simon McVittie from comment #31)
> Work in progress, might not work perfectly yet:
> 
> http://cgit.freedesktop.org/~smcv/dbus/log/?h=wip-dbussocket

I mentioned above and on Bug #89450 that this is kinda unfinished. I'm quite busy with non-D-Bus right now, but will try to come back to it (and in particular, work out why you get those compiler errors but I don't).

"Rename _dbus_full_duplex_pipe() to more descriptive name _dbus_socketpair()" is good to apply to master whenever, as far as I'm concerned.

"Fix assorted compiler warnings on Windows" and "cmake: only set CMP0053, CMP0054 on CMake >= 3.1" are not really anything to do with this bug, but the procedure I was using to test this branch was to build all of it in all my standard test configurations (I'm up to 12 now, including Autotools/native gcc with various configure options, Autotools/mingw gcc, CMake/native gcc and CMake/mingw gcc) and the warnings were getting in the way of seeing whether there was anything bad from this branch.

"Fix assorted compiler warnings on Windows" should ideally be several patches but I haven't got round to splitting it yet.

The rest of the branch shouldn't really be applied until all of it is, I don't think. At the beginning of the branch we're at some sort of stable state: we consistently (albeit incorrectly) treat sockets as if they were int. We know that this is wrong, but we also know that, in practice, it works OK.

At the end of the branch, we've reached another stable state: we consistently treat sockets as SOCKET, and the compiler enforces this for us by not allowing naive assignment between DBusSocket and int.

At all intermediate points (including after my modified version of your compiler-warning-squashing patch), there are still incorrect implicit conversions between int and SOCKET, and they're migrating around the source tree as I work through the changes. Sometimes they happen to be something that gcc warns about, sometimes they are something that it silently accepts - but they are there (and you can make that obvious by changing DBusSocket to not be assignment-compatible with int, as I did).

I do think this is a net improvement; you'll notice the last commit removes this comment

 * @todo Use for the file descriptors a struct
 * - struct DBusSocket{ int d; }; -
 * instead of int to get type-safety which
 * will be checked by the compiler.

which has been there since 2006 (even though back then, Havoc probably didn't know that the underlying type for SOCKET was uintptr_t).
Comment 35 Simon McVittie 2015-03-17 19:52:58 UTC
(In reply to Simon McVittie from comment #33)
> Hmm. Did you apply the whole branch?

To be clear, commit 84cb0ab7 is the one that I think should work.
Comment 36 Simon McVittie 2015-03-17 20:12:53 UTC
(In reply to Ralf Habacker from comment #32)
> x@y:~/src/dbus-smcv-build> make
> [  0%] Building C object
> dbus/CMakeFiles/dbus-1.dir/home/xxx/src/dbus-smcv/dbus/dbus-sysdeps-unix.o
> /home/xxx/src/dbus-smcv/dbus/dbus-sysdeps-unix.c: In function
> ‘_dbus_write_socket_two’:
> /home/xxx/src/dbus-smcv/dbus/dbus-sysdeps-unix.c:605:27: error: incompatible
> type for argument 1 of ‘_dbus_write_two’
>                            buffer2, start2, len2);
>                            ^

OK, found it. The bug is that the backwards-compatible code path for when you don't have MSG_NOSIGNAL was wrong, and the cmake build doesn't detect that yet. Commit 3690335 (which is meant to be squashed into the previous one) fixes that.

We might get a few more compilation failures similar to that on *BSD or old Linux, which I haven't tested; but there shouldn't be too many, and they should all be in platform-specific code where it's trivial to fix (fd -> fd.fd or similar).

> /home/xxx/src/dbus-smcv/dbus/dbus-sysdeps-unix.c:1740:51: warning:
> comparison between signed and unsigned integer expressions [-Wsign-compare]
>    if (strlen (_dbus_string_get_const_data (&buf)) != len)
>                                                    ^

I'm not sure why you get this: you added the SOCKLEN_T check recently, so cmake should now know that socklen_t exists?
Comment 37 Ralf Habacker 2015-03-24 07:14:44 UTC
Comment on attachment 114092 [details] [review]
Rename _dbus_full_duplex_pipe() to more descriptive name _dbus_socketpair().

applied to master from the smcv/wip-dbussocket branch
Comment 38 Simon McVittie 2015-03-31 12:13:04 UTC
Created attachment 114755 [details] [review]
[01/11] Use typedef DBusSocket for sockets fd's to avoid  conversion warnings

From: Ralf Habacker <ralf.habacker@freenet.de>

[smcv: remove unneeded and invalid dbus-sysdeps.h from public header;
make prototype of _dbus_socketpair() consistent; undo conversion
of getaddrinfo result from int to SOCKET; don't call
_dbus_return_val_if_fail() from internal function]
Comment 39 Simon McVittie 2015-03-31 12:13:16 UTC
Created attachment 114756 [details] [review]
[02/11] bus_unix_fds_passing_test: the results of  _dbus_socketpair are sockets
Comment 40 Simon McVittie 2015-03-31 12:13:48 UTC
Created attachment 114757 [details] [review]
[03/11] dbus-sysdeps: add more infrastructure around DBusSocket

This is all trivial right now, but will become significant when we
change DBusSocket into a type-safe struct.

---

See patch 11 for what this is going to turn into.
Comment 41 Simon McVittie 2015-03-31 12:14:37 UTC
Created attachment 114758 [details] [review]
[04/11] Mostly remove the remnants of an older socket  abstraction layer

This is only used on Windows, and wasn't even a particularly abstract
abstraction.

I've removed DBUS_SOCKET_IS_INVALID in favour of DBUS_SOCKET_IS_VALID
because I prefer to avoid double-negatives.
Comment 42 Simon McVittie 2015-03-31 12:15:21 UTC
Created attachment 114759 [details] [review]
[05/11] DBusMainLoop, DBusSocketSet: work in terms of  DBusPollable

This requires generic support for keying hash tables by DBusPollable:
there are already implementations for int and uintptr_t keys, but not
for "int or uintptr_t depending on platform", which is what
DBusPollable now means.
Comment 43 Simon McVittie 2015-03-31 12:16:06 UTC
Created attachment 114760 [details] [review]
[06/11] main: reload_pipe is (despite its name) a socket pair

---

One day I might rename it to reload_socket, or make it actually a pipe(), since it's entirely Unix-specific... but for now, RESOLVED LATER.
Comment 44 Simon McVittie 2015-03-31 12:16:33 UTC
Created attachment 114761 [details] [review]
[07/11] Split _dbus_set_fd_nonblocking vs.  _dbus_set_socket_nonblocking

The former is Unix-specific, the latter is also portable to Windows.
On Unix, they're really the same thing.
Comment 45 Simon McVittie 2015-03-31 12:16:48 UTC
Created attachment 114762 [details] [review]
[08/11] generic socket transport code: work in terms of  DBusSocket
Comment 46 Simon McVittie 2015-03-31 12:17:06 UTC
Created attachment 114763 [details] [review]
[09/11] Convert miscellaneous socket APIs to DBusSocket
Comment 47 Simon McVittie 2015-03-31 12:17:51 UTC
Created attachment 114764 [details] [review]
[10/11] Remove _dbus_socket_is_invalid, no longer used

It didn't have many users anyway, and I've replaced them with the
DBUS_SOCKET_IS_VALID macro.

---

Again, I reversed the sense because I don't not dislike double-negatives. :-)
Comment 48 Simon McVittie 2015-03-31 12:20:27 UTC
Created attachment 114765 [details] [review]
[11/11] Turn DBusSocket into a type-safe struct, preventing  inappropriate conversion

Fix the remaining platform-specific code to look at the struct's
appropriate platform-specific member.

---

This might cause minor and easily-fixed compilation failures on non-Linux, non-Windows platforms, if I've missed any uses of the raw fd... but I think on balance it's worth it anyway. If nothing else, that'll tell us who is testing D-Bus on their Unix platforms :-)
Comment 49 Thiago Macieira 2015-03-31 18:44:23 UTC
On OS X, it compiles and links without errors if --disable-developer. With --enable-developer, the -Werror warnings make the build totally impossible, though they don't look specific to this branch. Three tests failed with the error:

Failed to start message bus: launchd's environment variable DBUS_LAUNCHD_SESSION_BUS_SOCKET is empty, but should contain a socket path.

I don't know if it's caused by this.

On FreeBSD, I get the same warnings (clang with -Wcast-align). The three tests that failed with OS X do pass on FreeBSD. But test-fdpass (which was skipped on OS X) fails on FreeBSD:

/relay: ..**********..OK
/limit: ..**********..OK
/too-many/plus1: ..**********..OK
/too-many/plus2: ..**********..OK
/too-many/plus17: ..**********..OK
/too-many/split: ..**********..OK
/flood/1: ..**********...OK
/flood/half-limit: ..**********....OK
/flood/over-half-limit: ..**********.....OK
/flood/limit: ..**********......OK
/odd-limit/minus1: ..**********..OK
/odd-limit/at: ..**********...**
ERROR:fdpass.c:742:test_odd_limit: assertion failed: (dbus_message_contains_unix_fds (incoming))
FAIL test-fdpass (exit status: 134)
Comment 50 Thiago Macieira 2015-03-31 18:52:04 UTC
Testing master branch: exact same results, so none of the failures appear to be introduced by this branch.
Comment 51 Simon McVittie 2015-03-31 19:37:23 UTC
(In reply to Thiago Macieira from comment #50)
> Testing master branch: exact same results, so none of the failures appear to
> be introduced by this branch.

Please open separate bugs for those. Thanks for testing!

I would expect failures caused by this branch to be build failures, either from -Werror or actual hard failures.
Comment 52 Ralf Habacker 2015-03-31 19:55:44 UTC
Created attachment 114788 [details] [review]
[01/11] Use typedef DBusSocket for sockets fd's to avoid conversion warnings (rebased)
Comment 53 Ralf Habacker 2015-03-31 20:23:57 UTC
Comment on attachment 114756 [details] [review]
[02/11] bus_unix_fds_passing_test: the results of  _dbus_socketpair are sockets

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

looks good
Comment 54 Ralf Habacker 2015-03-31 20:26:21 UTC
Comment on attachment 114757 [details] [review]
[03/11] dbus-sysdeps: add more infrastructure around DBusSocket

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

looks good
Comment 55 Thiago Macieira 2015-03-31 20:35:18 UTC
(In reply to Simon McVittie from comment #51)
> (In reply to Thiago Macieira from comment #50)
> > Testing master branch: exact same results, so none of the failures appear to
> > be introduced by this branch.
> 
> Please open separate bugs for those. Thanks for testing!
> 
> I would expect failures caused by this branch to be build failures, either
> from -Werror or actual hard failures.

I had to compile without -Werror otherwise it wouldn't get past dbus-marshal.c, as clang is apparently stricter when casting from char* to higher-ranked pointer types. So it's possible that this patchset is introducing new warnings, but I doubt it. Almost everything I saw was either -Wcast-align or one unused function warning.

But no hard compilation failures and no new test errors. I will report the OS X and FreeBSD errors.
Comment 56 Ralf Habacker 2015-03-31 20:47:18 UTC
Comment on attachment 114758 [details] [review]
[04/11] Mostly remove the remnants of an older socket  abstraction layer

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

looks good
Comment 57 Simon McVittie 2015-04-01 11:15:54 UTC
(In reply to Ralf Habacker from comment #52)
> Created attachment 114788 [details] [review]
> [01/11] Use typedef DBusSocket for sockets fd's to avoid conversion warnings
> (rebased)

What did you change?
Comment 58 Ralf Habacker 2015-04-01 12:18:08 UTC
(In reply to Simon McVittie from comment #57)
> (In reply to Ralf Habacker from comment #52)
> > Created attachment 114788 [details] [review] [review]
> > [01/11] Use typedef DBusSocket for sockets fd's to avoid conversion warnings
> > (rebased)
> 
> What did you change?

The previous commit contained unrelated doc change revert from the following commit. 

commit 73af0d5d5c953d157e89f6c02d61872a0c490edf
Date:   Fri Mar 6 08:09:57 2015 +0100

    Rename _dbus_full_duplex_pipe() to more descriptive name _dbus_socketpair().
Comment 59 Simon McVittie 2015-04-01 12:55:48 UTC
(In reply to Ralf Habacker from comment #58)
> The previous commit contained unrelated doc change revert from the following
> commit. 
> 
> commit 73af0d5d5c953d157e89f6c02d61872a0c490edf
> Date:   Fri Mar 6 08:09:57 2015 +0100
> 
>     Rename _dbus_full_duplex_pipe() to more descriptive name
> _dbus_socketpair().

Good point. Your version looks better, and is ready to apply when we are happy with the rest of my branch (but we should still not apply it until the rest of the branch is ready, otherwise we're just moving the problem around).
Comment 60 Simon McVittie 2015-04-16 12:31:18 UTC
Any further thoughts on this? AFAICS, you've replaced patch 1, approved 2-4, and 5+ still need review.

Your replacement version of patch 1 looks good, but please don't merge it until we are ready to merge the whole series: until we get to patch 11, we aren't really enforcing the distinction between int and DBusSocket, so the problem is just moving around the codebase. We know that the current state works in practice (even though it's wrong in theory), and I'm pretty confident that after patch 11 we're in good shape (because the compiler stops us from getting it wrong), but the intermediate states are less good.
Comment 61 Ralf Habacker 2015-04-16 19:42:11 UTC
Comment on attachment 114760 [details] [review]
[06/11] main: reload_pipe is (despite its name) a socket pair

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

looks good
Comment 62 Ralf Habacker 2015-04-16 19:46:08 UTC
Comment on attachment 114761 [details] [review]
[07/11] Split _dbus_set_fd_nonblocking vs.  _dbus_set_socket_nonblocking

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

looks good otherwise

::: dbus/dbus-sysdeps.h
@@ +228,5 @@
>                                 DBusError      *error);
>  DBusSocket _dbus_accept       (DBusSocket      listen_fd);
>  
> +dbus_bool_t _dbus_set_socket_nonblocking (DBusSocket  fd,
> +                                          DBusError  *error);

this is a duplicate of the definition in line 167
Comment 63 Ralf Habacker 2015-04-16 20:02:52 UTC
Comment on attachment 114762 [details] [review]
[08/11] generic socket transport code: work in terms of  DBusSocket

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

looks good

::: dbus/dbus-connection.c
@@ +5149,1 @@
>                             int                         *fd)

I assume 'int' is used because of locked public api.
Comment 64 Ralf Habacker 2015-04-16 20:05:02 UTC
Comment on attachment 114763 [details] [review]
[09/11] Convert miscellaneous socket APIs to DBusSocket

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

looks good
Comment 65 Ralf Habacker 2015-04-16 20:05:36 UTC
Comment on attachment 114764 [details] [review]
[10/11] Remove _dbus_socket_is_invalid, no longer used

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

looks good
Comment 66 Ralf Habacker 2015-04-16 21:18:00 UTC
Created attachment 115135 [details] [review]
Convert mostly DBUS_SOCKET_... and DBUS_POLLABLE_.. macros to inline functions for more type safety.
Comment 67 Ralf Habacker 2015-04-16 21:23:34 UTC
Comment on attachment 114759 [details] [review]
[05/11] DBusMainLoop, DBusSocketSet: work in terms of  DBusPollable

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

a few api usage issues, otherwise looks good.

::: dbus/dbus-watch.c
@@ +317,2 @@
>                           watch_flags_to_string (dbus_watch_get_flags (link->data)),
> +                         DBUS_POLLABLE_PRINTABLE (watch->fd));

not better DBUS_POLLABLE_PRINTABLE (dbus_watch_get_pollable (link->data)) ?

@@ +333,3 @@
>                    
> +                  _dbus_verbose ("Removing watch on fd %" DBUS_POLLABLE_FORMAT " using newly-set remove function because initial add failed\n",
> +                                 DBUS_POLLABLE_PRINTABLE (watch2->fd));

not better DBUS_POLLABLE_PRINTABLE (dbus_watch_get_pollable (link2->data)) ?

@@ +389,5 @@
>  
>    if (watch_list->add_watch_function != NULL)
>      {
> +      _dbus_verbose ("Adding watch on fd %" DBUS_POLLABLE_FORMAT "\n",
> +                     DBUS_POLLABLE_PRINTABLE (watch->fd));

dito

@@ +420,5 @@
>    
>    if (watch_list->remove_watch_function != NULL)
>      {
> +      _dbus_verbose ("Removing watch on fd %" DBUS_POLLABLE_FORMAT "\n",
> +                     DBUS_POLLABLE_PRINTABLE (watch->fd));

dito

@@ +453,5 @@
>    if (watch_list->watch_toggled_function != NULL)
>      {
> +      _dbus_verbose ("Toggling watch %p on fd %" DBUS_POLLABLE_FORMAT " to %d\n",
> +                     watch,
> +                     DBUS_POLLABLE_PRINTABLE (watch->fd),

dito

@@ +669,5 @@
>  {
>    _dbus_return_if_fail (watch != NULL);
>  
> +  _dbus_verbose ("Setting watch fd %" DBUS_POLLABLE_FORMAT " data to data = %p function = %p from data = %p function = %p\n",
> +                 DBUS_POLLABLE_PRINTABLE (watch->fd),

dito

@@ +738,5 @@
>  
>    if (flags == 0)
>      {
> +      _dbus_verbose ("After sanitization, watch flags on fd %" DBUS_POLLABLE_FORMAT " were 0\n",
> +                     DBUS_POLLABLE_PRINTABLE (watch->fd));

dito
Comment 68 Ralf Habacker 2015-04-16 21:26:51 UTC
Comment on attachment 114757 [details] [review]
[03/11] dbus-sysdeps: add more infrastructure around DBusSocket

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

found platform related code ordering issue afterwards

::: dbus/dbus-sysdeps.h
@@ +349,4 @@
>  #define _DBUS_POLLNVAL    0x0020
>  #endif
>  
> +#ifdef DBUS_WIN

should be #ifndef DBUS_WIN first similiar to line 135
Comment 69 Ralf Habacker 2015-04-16 21:29:24 UTC
Comment on attachment 115135 [details] [review]
Convert mostly DBUS_SOCKET_... and DBUS_POLLABLE_.. macros to inline functions for more type safety.

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

::: dbus/dbus-sysdeps.h
@@ +370,4 @@
>  # define DBUS_POLLABLE_FORMAT "Iu"
> +
> +static inline DBusPollable
> +_dbus_socket_get_pollable(DBusSocket s) { return s; }

self review: space after function name missing
Comment 70 Simon McVittie 2015-04-17 10:23:05 UTC
(In reply to Ralf Habacker from comment #68)
> should be #ifndef DBUS_WIN first similiar to line 135

I think I see your point - are you saying that functionally, the code is fine, but stylistically, each of several conditional blocks should be in a consistent order? - but I disagree about the correct resolution.

For the existing #ifdef, I kept the "#ifndef DBUS_WIN" first because it was already like that, but as a general principle I prefer to reduce the number of negatives - I think #ifdef...#else is much less confusing than #ifndef...#else.

In all the places where it matters in dbus, the progression is fairly clearly "Windows is the special case, Unix is normal" - DBUS_UNIX covers Linux, Mac, *BSD, etc., and we do not currently support any non-Windows non-Unix platforms. So I prefer to have

#ifdef DBUS_WIN
  printf ("Windows\n");
#else
  printf ("not Windows\n");
#endif

rather than #ifndef.

If we do any re-ordering here, I would prefer to turn

> #ifndef DBUS_WIN
> typedef int DBusSocket;
> /* etc. */
> #else /* DBUS_WIN */
> typedef SOCKET DBusSocket;
> /* etc. */
> #endif

into

> #ifdef DBUS_WIN
> typedef SOCKET DBusSocket;
> /* etc. */
> #else /* not DBUS_WIN */
> typedef int DBusSocket;
> /* etc. */
> #endif
Comment 71 Simon McVittie 2015-04-17 10:32:03 UTC
Comment on attachment 115135 [details] [review]
Convert mostly DBUS_SOCKET_... and DBUS_POLLABLE_.. macros to inline functions for more type safety.

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

::: dbus/dbus-sysdeps.h
@@ +132,4 @@
>  # define DBUS_SOCKET_INIT { -1 }
> +
> +static inline int
> +_dbus_socket_printable (DBusSocket s) { return s.fd; }

This is a pretty small improvement, which is why I didn't do it like this myself: if you mistakenly pass an int to _dbus_socket_printable(), or whatever, the compiler is already going to stop you. The only extra type-safety this gives you is to stop you from using some other struct that happens to have a member named fd.

Having said that, sure, it's still an improvement. We can add this to the branch. (I'll test it on my collection of build configurations before merging.)

@@ +134,5 @@
> +static inline int
> +_dbus_socket_printable (DBusSocket s) { return s.fd; }
> +
> +static inline dbus_bool_t
> +_dbus_socket_is_valid (DBusSocket s) { return s.fd >= 0; }

I prefer to put logical conditions in parentheses when they're used in an expression, to make the interpretation obvious:

    return (s.fd >= 0)

but for inline functions this small, it isn't important.
Comment 72 Simon McVittie 2015-04-17 10:53:44 UTC
Comment on attachment 114759 [details] [review]
[05/11] DBusMainLoop, DBusSocketSet: work in terms of  DBusPollable

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

::: dbus/dbus-watch.c
@@ +317,2 @@
>                           watch_flags_to_string (dbus_watch_get_flags (link->data)),
> +                         DBUS_POLLABLE_PRINTABLE (watch->fd));

This is within watch.c, at the same level of abstraction as _dbus_watch_get_pollable() itself, so I don't think there's any value in that indirection. At worst, it costs us an external function call; at best, it costs nothing (if the compiler works out that it can be inlined[1]), but is longer and doesn't provide any benefit.

I suspect that the major reason for using dbus_watch_get_flags() here was that whoever added it didn't want to declare "DBusWatch *watch = link->data" just so that they could say watch->flags. dbus_watch_get_flags() also has an extra assertion, so I suppose checking that assertion might have some small amount of value.

[1] on Linux and other ELF platforms, calls to extern functions can never be inlined unless you use nonstandard linker options (-Bsymbolic), because of support for the rarely-used "function interposition" feature. I've occasionally wondered whether to set -Bsymbolic for dbus.
Comment 73 Ralf Habacker 2015-04-17 12:50:42 UTC
(In reply to Simon McVittie from comment #70)
> (In reply to Ralf Habacker from comment #68)
> > should be #ifndef DBUS_WIN first similiar to line 135
> 
> I think I see your point - are you saying that functionally, the code is
> fine, but stylistically, each of several conditional blocks should be in a
> consistent order? 

you got it
Comment 74 Simon McVittie 2015-04-17 13:13:04 UTC
(In reply to Ralf Habacker from comment #62)
> this is a duplicate of the definition in line 167

Good point. Testing a fixed version

(In reply to Ralf Habacker from comment #63)
> ::: dbus/dbus-connection.c
> @@ +5149,1 @@
> >                             int                         *fd)
> 
> I assume 'int' is used because of locked public api.

Yes.

(In reply to Simon McVittie from comment #70)
> If we do any re-ordering here, I would prefer to turn
> 
> > #ifndef DBUS_WIN
> > typedef int DBusSocket;
> > /* etc. */
> 
> into
> 
> > #ifdef DBUS_WIN
> > typedef SOCKET DBusSocket;
> > /* etc. */

Added a patch that does this; testing now.
Comment 75 Simon McVittie 2015-04-17 13:13:51 UTC
Created attachment 115157 [details] [review]
[fixup for 07/11] remove duplicate declaration of  _dbus_set_socket_nonblocking
Comment 76 Simon McVittie 2015-04-17 13:14:52 UTC
Created attachment 115158 [details] [review]
[fixup for Attachment #115135 [details]] style: space before parenthesis in declarations  and definitions

---

I've assumed Attachment #115135 [details] is meant to apply at the end of my patch series.
Comment 77 Simon McVittie 2015-04-17 13:17:15 UTC
Created attachment 115159 [details] [review]
[13] DBusSocket: put the #ifdef case before the !defined  case

This avoids the confusing #ifndef...#else anti-pattern.

---

To be applied after Attachment #115135 [details] and Attachment #115158 [details] (if applied out-of-order, they would conflict).

Probably obvious, but the ones labelled "fixup" are intended to be squashed into the previous one in the patch series. This one is not; it is its own commit.
Comment 78 Simon McVittie 2015-04-17 13:18:32 UTC
(In reply to Ralf Habacker from comment #67)
> not better DBUS_POLLABLE_PRINTABLE (dbus_watch_get_pollable (link->data)) ?

I have not done anything about this: I hope my response above, explaining why not, is enough to address it.

If you think I'm wrong, we can discuss further.
Comment 79 Simon McVittie 2015-04-17 13:20:03 UTC
http://cgit.freedesktop.org/~smcv/dbus/log/?h=dbussocket if you'd prefer to review like that.
Comment 80 Ralf Habacker 2015-04-22 09:39:20 UTC
Comment on attachment 114765 [details] [review]
[11/11] Turn DBusSocket into a type-safe struct, preventing  inappropriate conversion

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

Reviewing in platform related files is easy, in shared code I had to inspect the source code from the dbussocket branch to see for which platform the related changes is targeted.

looks good.

::: bus/dispatch.c
@@ +5144,2 @@
>      _dbus_assert_not_reached("Failed to read value from pipe.");
>  

this is in bus_unix_fds_passing_test() and therefore unix only: okay

::: bus/main.c
@@ +70,4 @@
>          char action[2] = { ACTION_RELOAD, '\0' };
>  
>          _dbus_string_init_const (&str, action);
> +        if ((reload_pipe[RELOAD_WRITE_END].fd > 0) &&

this is in signal_handler, which is unix only: okay

@@ +103,4 @@
>          DBusString str;
>          char action[2] = { ACTION_QUIT, '\0' };
>          _dbus_string_init_const (&str, action);
> +        if ((reload_pipe[RELOAD_WRITE_END].fd < 0) ||

dito

@@ +248,4 @@
>    while (!_dbus_string_init (&str))
>      _dbus_wait_for_memory ();
>  
> +  if ((reload_pipe[RELOAD_READ_END].fd > 0) &&

unix only: okay

::: dbus/dbus-watch.c
@@ +613,5 @@
> +  s.fd = watch->fd;
> +#else
> +  s = watch->fd;
> +#endif
> +

looks ugly, but is caused by different implementations of DBusPollable (unix:int, windows:DBusSocket)
Having unix variant of DBusPollable also derived from DBusSocket and adding some _dbus_pollable_... functions would have the benefit of a common api and would also reduce the required number of #ifdef DBUS_UNIX ... #else ... #endif.

::: test/fdpass.c
@@ +540,4 @@
>    /* This is blatant cheating, and the API documentation specifically
>     * tells you not use this function in this way. Never do this
>     * in application code. */
> +  if (!dbus_connection_get_socket (f->left_client_conn,

unix only: looks good
Comment 81 Ralf Habacker 2015-04-22 09:41:36 UTC
Comment on attachment 115159 [details] [review]
[13] DBusSocket: put the #ifdef case before the !defined  case

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

looks good
Comment 82 Ralf Habacker 2015-04-22 09:42:01 UTC
Comment on attachment 115157 [details] [review]
[fixup for 07/11] remove duplicate declaration of  _dbus_set_socket_nonblocking

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

looks good
Comment 83 Ralf Habacker 2015-04-22 09:42:17 UTC
Comment on attachment 115158 [details] [review]
[fixup for Attachment #115135 [details]] style: space before parenthesis in declarations  and definitions

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

looks good
Comment 84 Simon McVittie 2015-04-22 11:20:39 UTC
(In reply to Ralf Habacker from comment #80)
> ::: dbus/dbus-watch.c
> @@ +613,5 @@
> > +  s.fd = watch->fd;
> > +#else
> > +  s = watch->fd;
> > +#endif
> > +
> 
> looks ugly, but is caused by different implementations of DBusPollable
> (unix:int, windows:DBusSocket)
> Having unix variant of DBusPollable also derived from DBusSocket and adding
> some _dbus_pollable_... functions would have the benefit of a common api and
> would also reduce the required number of #ifdef DBUS_UNIX ... #else ...
> #endif.

It would be incorrect for the Unix DBusPollable to be derived from DBusSocket: on Windows, select() (and hence _dbus_poll()) only works on SOCKETs, but on Unix, any fd is (at least potentially) pollable. In particular, on Unix you can select() on a pipe, but on Windows you can't. We rely on this in the Unix-specific files dbus-spawn.c, where we poll a pipe, and bus/dir-watch-{inotify,kqueue}.c, where we poll the appropriate OS-specific fd.

Also, as I said in the commit message, DBusPollable needs to be just int, not a struct, because the way the Unix _dbus_poll() implementation works is that we need to make sure that DBusPollFD has the same in-memory layout as native struct pollfd. On Windows, there's no poll() to be compatible with, so we're free to put whatever we want in the DBusPollFD struct; I chose to use a DBusSocket.

So these particular #ifdefs are not harmful, IMO: they are there because Unix and Windows really do work differently in this respect.

Similarly, the stuff in dbus-hash.h really does need to distinguish between the two platforms, because the thing we're using as a hash key has a different size (on 64-bit).



As a future change, we could consider refactoring polling so the Windows code can stop pretending to have a poll() implementation:

* add a Windows-specific dbus-socket-set-winsock.c which implements
  the DBusSocketSet interface using select() directly, just like -epoll.c
  uses Linux epoll directly; stop building any other DBusSocketSet backends
  on Windows, which lets us treat -poll.c as Unix-specific

* add _dbus_socket_poll() which polls exactly one socket
  (using _dbus_poll() on Unix and select() on Windows), and use that in
  dbus-transport-socket.c, something like this:

      DBusWatchFlags _dbus_socket_poll (DBusSocket sock,
          int timeout_milliseconds,
          DBusWatchFlags events);

  (or alternatively move the DBusSocketSet stuff into the shared library
  and use that, but I suspect that _dbus_socket_poll() would be really
  simple on both Windows and Unix)

* make _dbus_poll() and DBusPollFD Unix-specific, or just use poll()
  and struct pollfd directly (we do not currently support any non-Windows
  platforms without poll() and struct pollfd), in dbus-socket-set-poll.c,
  _dbus_socket_poll_one(), and the other remaining user of _dbus_poll(),
  which is the Unix-specific dbus-spawn.c

* ... that means we can change DBusPollable from int to struct { int }
  on Unix, if desired

But I think that's too large a change to block this branch while waiting for it, and if we want to do this it deserves its own bug#.
Comment 85 Simon McVittie 2015-05-05 11:04:10 UTC
Is there anything here that you strongly object to?

There are a few points where I have responded, but not changed the code. Do you agree with my reasoning on these or do we need to discuss them further / work out a concrete change that would be mergeable?

(In reply to Simon McVittie from comment #78)
> (In reply to Ralf Habacker from comment #67)
> > not better DBUS_POLLABLE_PRINTABLE (dbus_watch_get_pollable (link->data)) ?
> 
> I have not done anything about this: I hope my response above, explaining
> why not, is enough to address it.

(In reply to Simon McVittie from comment #84)
> It would be incorrect for the Unix DBusPollable to be derived from
> DBusSocket
...
> So these particular #ifdefs are not harmful, IMO: they are there because
> Unix and Windows really do work differently in this respect.

> As a future change, we could consider refactoring polling so the Windows
> code can stop pretending to have a poll() implementation:
...
> But I think that's too large a change to block this branch while waiting for
> it, and if we want to do this it deserves its own bug#.
Comment 86 Simon McVittie 2015-05-05 11:13:59 UTC
(In reply to Simon McVittie from comment #85)
> > As a future change, we could consider refactoring polling so the Windows
> > code can stop pretending to have a poll() implementation:
> ...
> > But I think that's too large a change to block this branch while waiting for
> > it, and if we want to do this it deserves its own bug#.

I've opened Bug #90314 for this part.
Comment 87 Simon McVittie 2015-05-12 19:06:29 UTC
(In reply to Simon McVittie from comment #85)
> Is there anything here that you strongly object to?
> 
> There are a few points where I have responded, but not changed the code. Do
> you agree with my reasoning on these or do we need to discuss them further /
> work out a concrete change that would be mergeable?

I've pushed this as-is (with the fixups applied, obviously). I think we both agree that this branch is better than the previous state, even if there's more that could be done.

If there's stuff that you object to, please reopen this (or open a new bug, either works) and suggest how I can fix it.

Otherwise: fixed in git for 1.9.16.


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.