Re: [PATCH 2/7] dpwsockx: Implementation of SPInit

[Henri Verbeet]

2009/9/1 Ismael Barros :
> +typedef struct tagDPSP_MSG_HEADER
> +{
> - DWORD       size;           /* size & 0x000FFFFF, token & 0xFFF00000 */
> - SOCKADDR_IN SockAddr;
>
> +} DPSP_MSG_HEADER, *LPDPSP_MSG_HEADER;
> +typedef const DPSP_MSG_HEADER* LPCDPSP_MSG_HEADER;
> -

It seems you're sending this over the network, so you should make sure
it's correctly packed, and take the byte order into account.

[PATCH 2/7] dpwsockx: Implementation of SPInit

[Ismael Barros²]

On Tue, Sep 1, 2009 at 11:00 AM, Henri Verbeet wrote:
> 2009/9/1 Ismael Barros :
> > +typedef struct tagDPSP_MSG_HEADER
> > +{
> > - DWORD       size;           /* size & 0x000FFFFF, token & 0xFFF00000 */
> > - SOCKADDR_IN SockAddr;
> >
> > +} DPSP_MSG_HEADER, *LPDPSP_MSG_HEADER;
> > +typedef const DPSP_MSG_HEADER* LPCDPSP_MSG_HEADER;
> > -
>
> It seems you're sending this over the network, so you should make sure
> it's correctly packed, and take the byte order into account.

I tested it with a wireshark dissector and against the original
implementation, and it seems to work nicely.

[PATCH 2/7] dpwsockx: Implementation of SPInit

[Henri Verbeet]

2009/9/1 Ismael Barros² :
> On Tue, Sep 1, 2009 at 11:00 AM, Henri Verbeet wrote:
> > 2009/9/1 Ismael Barros :
> > > +typedef struct tagDPSP_MSG_HEADER
> > > +{
> > > - DWORD       size;           /* size & 0x000FFFFF, token & 0xFFF00000 */
> > > - SOCKADDR_IN SockAddr;
> > >
> > > +} DPSP_MSG_HEADER, *LPDPSP_MSG_HEADER;
> > > +typedef const DPSP_MSG_HEADER* LPCDPSP_MSG_HEADER;
> > > -
> >
> > It seems you're sending this over the network, so you should make sure
> > it's correctly packed, and take the byte order into account.
>
> I tested it with a wireshark dissector and against the original
> implementation, and it seems to work nicely.

Probably, but that depends on how the compiler packs the structure,
and on the host byte order.

[PATCH 2/7] dpwsockx: Implementation of SPInit

[Ismael Barros²]

On Tue, Sep 1, 2009 at 12:43 PM, Henri Verbeet wrote:
> 2009/9/1 Ismael Barros² :
> > Is there any standard way to deal with this cases? Any example in the codebase?
>
> For packing, look for "#include <pshpack1.h>" and "#include
> <poppack.h>" in some of the headers. For byte ordering look at how
> htonl etc. are defined in include/winsock.h, but you'll want the
> opposite since the data uses x86 byte ordering.

I've been looking into iphlpapi/ip.h (just learned bit fields
exist...); would this implementation be fine?

#include "pshpack1.h"

typedef struct tagDPSP_MSG_HEADER
{
#ifdef WORDS_BIGENDIAN
    DWORD size:20;
    DWORD token:12;
#else
    DWORD token:12;
    DWORD size:20;
#endif
    SOCKADDR_IN SockAddr;
} DPSP_MSG_HEADER, *LPDPSP_MSG_HEADER;
typedef const DPSP_MSG_HEADER* LPCDPSP_MSG_HEADER;

/* DPSP_MSG_HEADER->token */
#define DPSP_MSG_TOKEN_REMOTE    0xFAB
#define DPSP_MSG_TOKEN_FORWARDED 0xCAB
#define DPSP_MSG_TOKEN_SERVER    0xBAB

#include "poppack.h"

[PATCH 2/7] dpwsockx: Implementation of SPInit

[Henri Verbeet]

2009/9/4 Ismael Barros² :
> I've been looking into iphlpapi/ip.h (just learned bit fields
> exist...); would this implementation be fine?
>
> #include "pshpack1.h"
>
> typedef struct tagDPSP_MSG_HEADER
> {
> #ifdef WORDS_BIGENDIAN
>    DWORD size:20;
>    DWORD token:12;
> #else
>    DWORD token:12;
>    DWORD size:20;
> #endif
>    SOCKADDR_IN SockAddr;
> } DPSP_MSG_HEADER, *LPDPSP_MSG_HEADER;

Probably not. I'm not sure iphlpapi/ip.h does things right in the
first place, it seems to confuse bitfield ordering with machine byte
ordering. As a general rule, I think it's best to avoid bitfields for
things like writing data to a file or sending data over the network,
it's just a pain. Aside from that, using the bitfields in this way
messes with the bit ordering, but doesn't change the byte order. For
the bitfields in iphlpapi/ip.h that's not an issue because they fit in
a single byte. Just store the data in a DWORD with the appropriate
masks and shifts, and byte swap that DWORD when reading/writing it.

[PATCH 2/7] dpwsockx: Implementation of SPInit

[Ismael Barros²]

On Fri, Sep 4, 2009 at 12:12 PM, Henri Verbeet wrote:
> 2009/9/4 Ismael Barros² :
> > I've been looking into iphlpapi/ip.h (just learned bit fields
> > exist...); would this implementation be fine?
> >
> > #include "pshpack1.h"
> >
> > typedef struct tagDPSP_MSG_HEADER
> > {
> > #ifdef WORDS_BIGENDIAN
> >    DWORD size:20;
> >    DWORD token:12;
> > #else
> >    DWORD token:12;
> >    DWORD size:20;
> > #endif
> >    SOCKADDR_IN SockAddr;
> > } DPSP_MSG_HEADER, *LPDPSP_MSG_HEADER;
>
> Probably not. I'm not sure iphlpapi/ip.h does things right in the
> first place, it seems to confuse bitfield ordering with machine byte
> ordering. As a general rule, I think it's best to avoid bitfields for
> things like writing data to a file or sending data over the network,
> it's just a pain. Aside from that, using the bitfields in this way
> messes with the bit ordering, but doesn't change the byte order. For
> the bitfields in iphlpapi/ip.h that's not an issue because they fit in
> a single byte. Just store the data in a DWORD with the appropriate
> masks and shifts, and byte swap that DWORD when reading/writing it.

Here is the next try, how does it look?
It seems to behave correctly in some quick tests (byte-swapping
parameters to emulate big-endian).

Thanks a lot for the feedback, really appreciated.

-------------

#ifdef WORDS_BIGENDIAN

static inline u_short __dpws_ushort_swap(u_short s)
{
    return (s >> 8) | (s << 8);
}
static inline u_long __dpws_ulong_swap(u_long l)
{
    return ((u_long)__dpws_ushort_swap((u_short)l) << 16) |
__dpws_ushort_swap((u_short)(l >> 16));
}

#define dpws_letohl(l) __dpws_ulong_swap(l)
#define dpws_letohs(s) __dpws_ushort_swap(s)
#define dpws_htolel(l) __dpws_ulong_swap(l)
#define dpws_htoles(s) __dpws_ushort_swap(s)

#else /* WORDS_BIGENDIAN */

#define dpws_letohl(l) ((u_long)(l))
#define dpws_letohs(s) ((u_short)(s))
#define dpws_htolel(l) ((u_long)(l))
#define dpws_htoles(s) ((u_short)(s))

#endif /* WORDS_BIGENDIAN */

#include "pshpack1.h"

typedef struct tagDPSP_MSG_HEADER
{
    DWORD       mixed;
    SOCKADDR_IN SockAddr;
} DPSP_MSG_HEADER, *LPDPSP_MSG_HEADER;
typedef const DPSP_MSG_HEADER* LPCDPSP_MSG_HEADER;

#include "poppack.h"

#define DPSP_MSG_TOKEN_REMOTE    0xFAB00000
#define DPSP_MSG_TOKEN_FORWARDED 0xCAB00000
#define DPSP_MSG_TOKEN_SERVER    0xBAB00000

#define DPSP_MSG_MAKE_MIXED(s,t) dpws_htolel((s) | (t))
#define DPSP_MSG_SIZE(m)         dpws_letohl((m) & 0x000FFFFF)
#define DPSP_MSG_TOKEN(m)        ((m) & 0xFFF00000)

[PATCH 2/7] dpwsockx: Implementation of SPInit

[Henri Verbeet]

2009/9/9 Ismael Barros² :
> #ifdef WORDS_BIGENDIAN
>
> static inline u_short __dpws_ushort_swap(u_short s)
> {
>    return (s >> 8) | (s << 8);
> }
> static inline u_long __dpws_ulong_swap(u_long l)
> {
>    return ((u_long)__dpws_ushort_swap((u_short)l) << 16) |
> __dpws_ushort_swap((u_short)(l >> 16));
> }
>
> #define dpws_letohl(l) __dpws_ulong_swap(l)
> #define dpws_letohs(s) __dpws_ushort_swap(s)
> #define dpws_htolel(l) __dpws_ulong_swap(l)
> #define dpws_htoles(s) __dpws_ushort_swap(s)
>
> #else /* WORDS_BIGENDIAN */
>
> #define dpws_letohl(l) ((u_long)(l))
> #define dpws_letohs(s) ((u_short)(s))
> #define dpws_htolel(l) ((u_long)(l))
> #define dpws_htoles(s) ((u_short)(s))
>
> #endif /* WORDS_BIGENDIAN */
>
> #include "pshpack1.h"
>
> typedef struct tagDPSP_MSG_HEADER
> {
>    DWORD       mixed;
>    SOCKADDR_IN SockAddr;
> } DPSP_MSG_HEADER, *LPDPSP_MSG_HEADER;
> typedef const DPSP_MSG_HEADER* LPCDPSP_MSG_HEADER;
>
> #include "poppack.h"
>
> #define DPSP_MSG_TOKEN_REMOTE    0xFAB00000
> #define DPSP_MSG_TOKEN_FORWARDED 0xCAB00000
> #define DPSP_MSG_TOKEN_SERVER    0xBAB00000
>
> #define DPSP_MSG_MAKE_MIXED(s,t) dpws_htolel((s) | (t))
> #define DPSP_MSG_SIZE(m)         dpws_letohl((m) & 0x000FFFFF)
> #define DPSP_MSG_TOKEN(m)        ((m) & 0xFFF00000)

I think it should be like this:
#define DPSP_MSG_SIZE(m)         (dpws_letohl(m) & 0x000FFFFF)
#define DPSP_MSG_TOKEN(m)        (dpws_letohl(m) & 0xFFF00000)

I.e., swap before masking. Perhaps you just want to do this though:
#define DPSP_MSG_MAKE_MIXED(s,t) ((s) | (t))
#define DPSP_MSG_SIZE(m)         ((m) & 0x000fffff)
#define DPSP_MSG_TOKEN(m)        ((m) & 0xfff00000)
...
m = DPSP_MSG_MAKE_MIXED(s, t);
header.mixed = dpws_htolel(m);
...
m = dpws_letohl(header.mixed);
s = DPSP_MSG_SIZE(m);
t = DPSP_MSG_SIZE(m);

[PATCH 2/7] dpwsockx: Implementation of SPInit

[Kai Blin]

On Tuesday 01 September 2009 11:00:37 Henri Verbeet wrote:

I realize I'm a bit late for the party, but anyway:
> 2009/9/1 Ismael Barros :
> > +typedef struct tagDPSP_MSG_HEADER
> > +{
> > - DWORD       size;           /* size & 0x000FFFFF, token & 0xFFF00000
> >
> > */ +    SOCKADDR_IN SockAddr;
> > +} DPSP_MSG_HEADER, *LPDPSP_MSG_HEADER;
> > +typedef const DPSP_MSG_HEADER* LPCDPSP_MSG_HEADER;
> > -
>
> It seems you're sending this over the network, so you should make sure
> it's correctly packed, and take the byte order into account.

Wine runs on architectures with different byte order? I'm not sure if we need 
to be too concerned here. DirectPlay basically sets up a couple of structs 
for managing multiplayer games and pushes the structs over the network. It's 
a pretty common way for Microsoft to define "network protocols". I've even 
seen pointers in the structures being passed, of course they're completely 
meaningless to the other peers.

Of course the packing needs to match what MS is sending over the network.

Cheers,
Kai

[PATCH 2/7] dpwsockx: Implementation of SPInit

[Henri Verbeet]

2009/10/10 Kai Blin :
> Wine runs on architectures with different byte order? I'm not sure if we need
> to be too concerned here. DirectPlay basically sets up a couple of structs

We have at least some level of support for PowerPC, SPARC, Alpha and
ARM. Probably not something to be overly concerned about, no, but I
don't think maintaining the difference between network and host byte
order is a bad idea in general.

[PATCH 2/7] dpwsockx: Implementation of SPInit

[Kai Blin]

On Saturday 10 October 2009 12:08:32 Henri Verbeet wrote:
> 2009/10/10 Kai Blin :
> > Wine runs on architectures with different byte order? I'm not sure if we
> > need to be too concerned here. DirectPlay basically sets up a couple of
> > structs
>
> We have at least some level of support for PowerPC, SPARC, Alpha and
> ARM. Probably not something to be overly concerned about, no, but I
> don't think maintaining the difference between network and host byte
> order is a bad idea in general.

But I highly doubt DirectPlay works on any of those. It's always in intel 
byteorder. I don't disagree in general, but for DirectPlay, it's just not an 
issue whatsoever.

Cheers,
Kai

[PATCH 2/7] dpwsockx: Implementation of SPInit

[Henri Verbeet]

2009/10/10 Kai Blin :
> But I highly doubt DirectPlay works on any of those.

That would be a bug then, although perhaps not the most important one.
> It's always in intel byteorder.

That just means the network byte order is little endian.

I agree this isn't the most important thing in a practical sense, but
as a principle I think network protocol implementations should
maintain that distinction. The same applies for reading/writing data
from/to disk or any other external source/destination.

[PATCH 2/7] dpwsockx: Implementation of SPInit

[Kai Blin]

On Saturday 10 October 2009 18:18:30 Henri Verbeet wrote:
> 2009/10/10 Kai Blin :
> > But I highly doubt DirectPlay works on any of those.
>
> That would be a bug then, although perhaps not the most important one.
> > It's always in intel byteorder.
>
> That just means the network byte order is little endian.
>
> I agree this isn't the most important thing in a practical sense, but
> as a principle I think network protocol implementations should
> maintain that distinction.

I'd agree if DirectPlay was a networking protocol. It's not. It only works 
over the network by accident.
> The same applies for reading/writing data 
> from/to disk or any other external source/destination.

I just don't see why we need to be more compatible than native in this case, 
that's all.

Cheers,
Kai