client crashes when running server on arm5te

[AndersF]

Hi,
I recently deployed a new server running debian. It's an arm5TE based openrd client box.
Unfortunately the MVP and the Windows clients don't play very well with it.

The MVP client, for example, crashes when reading the recordings list after the printouts below:
I can se that there are htonl/ntohl in the client and server code, but I would like to know wther anyone has run the server on anything else than i86 before?

0:55:31.300471 [debug]  32 Remote - Button 21
10:55:31.311836 [debug]  32 BoxStack - Update called
10:55:31.312646 [debug]  32 BoxStack - Locked for update
10:55:31.339986 [debug]  32 BoxStack - Unlocked for update
10:55:31.754882 [debug]  32 Remote - Button 21
10:55:31.767367 [debug]  32 BoxStack - Update called
10:55:31.768219 [debug]  32 BoxStack - Locked for update
10:55:31.794675 [debug]  32 BoxStack - Unlocked for update
10:55:36.756600 [debug]  32 Remote - Button 37
10:55:36.757496 [debug]  32 Boxx - Construct, now 5
10:55:36.758289 [debug]  32 Boxx - Construct, now 6
10:55:36.797757 [debug]  32 TBBoxx - Draw
10:55:36.869361 [debug]  32 BoxStack - add called
10:55:36.870160 [debug]  32 BoxStack - Locked for add
10:55:36.870925 [debug]  32 BoxStack - Unlocked for add
10:55:36.871686 [debug]  32 BoxStack - Update called
10:55:36.872446 [debug]  32 BoxStack - Locked for update
10:55:36.957161 [debug]  32 BoxStack - Unlocked for update
10:55:36.958224 [debug]  32 VDR - RR sleep - opcode 2
10:55:36.961141 [debug]  37 VDR - Rxd a response packet, requestID=15, len=248
10:55:36.962242 [debug]  32 VDR - RR unsleep

BR
Anders

[MartenR]

I did run the server only on a i386 plattform. But I can tell from porting the client, that endian issues are a real nightmare! I would suggest that you set up a debugging environment were you inspect on client side which kind of date comes in. (E.g. Windows client ) and then you might see, whcih data element is not properly send by the server side.

Marten

[AndersF]

I found a solution to my problem!
An analysis  of the response from the server, showed that the last character and NULL of a string were overwritten by the following ULONG.
Locking at addULONG() of responsepacket.c it does:

*(ULONG*)&buffer[bufUsed]=htonl(ul);

replacing the clause with :

ULONG ulnw = htonl(ul);
memcpy(buffer+bufUsed, &ulnw, sizeof(ULONG) );

solved the problem

I guess that the left hand side casting of the first clause, for alignment reasons, is evaluated differently depending on processor arcitecture.
The addLONG() and addULLONG() need to modified in a similar way.

Would it be possible to get CVS updated with a patch?

BR
Anders

[AndersF]

Patch below:

anders@server1:/mnt/disk1/src/vdr-1.6.0/PLUGINS/src$ cat vompserver.patch
diff -Naur vompserver.org/responsepacket.c vompserver/responsepacket.c
--- vompserver.org/responsepacket.c   2009-11-20 05:34:28.075696779 +0000
+++ vompserver/responsepacket.c   2009-11-22 06:32:37.415696613 +0000
@@ -87,7 +87,8 @@
bool ResponsePacket::addULONG(ULONG ul)
{
   if (!checkExtend(sizeof(ULONG))) return false;
-  *(ULONG*)&buffer[bufUsed] = htonl(ul);
+  ULONG ulnw = htonl(ul);
+  memcpy(buffer + bufUsed, &ulnw, sizeof(ULONG));
   bufUsed += sizeof(ULONG);
   return true;
} 
@@ -103,7 +104,8 @@
bool ResponsePacket::addLONG(LONG l)
{
   if (!checkExtend(sizeof(LONG))) return false;
-  *(LONG*)&buffer[bufUsed] = htonl(l);
+  LONG lnw = htonl(l);
+  memcpy(buffer + bufUsed, &lnw, sizeof(LONG));
   bufUsed += sizeof(LONG);
   return true;
}
@@ -111,7 +113,8 @@
bool ResponsePacket::addULLONG(ULLONG ull)
{
   if (!checkExtend(sizeof(ULLONG))) return false;
-  *(ULLONG*)&buffer[bufUsed] = htonll(ull);
+  ULLONG ullnw = htonll(ull);
+  memcpy(buffer + bufUsed, &ullnw, sizeof(ULLONG));
   bufUsed += sizeof(ULLONG);
   return true;
}

[MartenR]

Is a memcpy really necessary, can you try:

*((ULONG*)(buffer+bufUsed))=htonl(ul);
instead.

Marten

[MarkC]

Is a memcpy really necessary, can you try:

*((ULONG*)(buffer+bufUsed))=htonl(ul);
instead.

Marten

I think this code is identical to the original by definition.

p[x] is defined as *(p+x), and I'm pretty sure &*p == p, at least in this case where p is an unsigned char*.

Even if I'm wrong and your code does work on ARM, I still think memcpy is the correct method, otherwise we're just guessing about portability. Pointer casting can be a nightmare: for example, what about a platform where unsigned long pointers have to be aligned?

[Edit: indeed, alignment is what AndersF suggests is the problem here]

It shouldn't really make a difference to the end product [on x86]: I expect that all modern compilers, on an optimised build, will replace the memcpy with a direct assignment if possible.

[meinetwegen]

Patch below:

anders@server1:/mnt/disk1/src/vdr-1.6.0/PLUGINS/src$ cat vompserver.patch
diff -Naur vompserver.org/responsepacket.c vompserver/responsepacket.c
--- vompserver.org/responsepacket.c   2009-11-20 05:34:28.075696779 +0000
+++ vompserver/responsepacket.c   2009-11-22 06:32:37.415696613 +0000
@@ -87,7 +87,8 @@
bool ResponsePacket::addULONG(ULONG ul)
{
   if (!checkExtend(sizeof(ULONG))) return false;
-  *(ULONG*)&buffer[bufUsed] = htonl(ul);
+  ULONG ulnw = htonl(ul);
+  memcpy(buffer + bufUsed, &ulnw, sizeof(ULONG));
   bufUsed += sizeof(ULONG);
   return true;
} 
@@ -103,7 +104,8 @@
bool ResponsePacket::addLONG(LONG l)
{
   if (!checkExtend(sizeof(LONG))) return false;
-  *(LONG*)&buffer[bufUsed] = htonl(l);
+  LONG lnw = htonl(l);
+  memcpy(buffer + bufUsed, &lnw, sizeof(LONG));
   bufUsed += sizeof(LONG);
   return true;
}
@@ -111,7 +113,8 @@
bool ResponsePacket::addULLONG(ULLONG ull)
{
   if (!checkExtend(sizeof(ULLONG))) return false;
-  *(ULLONG*)&buffer[bufUsed] = htonll(ull);
+  ULLONG ullnw = htonll(ull);
+  memcpy(buffer + bufUsed, &ullnw, sizeof(ULLONG));
   bufUsed += sizeof(ULLONG);
   return true;
}

Ist this the complete patch? It did not work for me. Actually I have seen this dubious type punning pointer pattern all over the code. There must be a lot more places where this is a problem.