Change producing connections from UDP to raw sockets

[MartinMelikMerkumians]

EtherNet/IP mandates, that all messages shall be sent form a specified port (default 2222). In order to enforce this source port, the best solution is to bypass the UDP protocol and construct the UDP header in OpENer

[jkvatne]

This seems to be unnecessary. At least on Windows, I can make the UDP source port equal to 2222 by calling bind() before transmission (In CreateUdpSocket). The use of Raw Sockets is not allowed on Windows unless you have administrator rights, which is not easy to obtain unless the program is started with it. Any way, we should avoid running as administrator. Also the new UDP module uses #include <netinet/in.h>, which does not exist on Windows. The required code to be inserted in generic_networkhandler.c:876 s is:

    struct sockaddr_in source_addr = {.sin_addr = INADDR_ANY, .sin_family = AF_INET, .sin_port = htons(0x08ae)};
    bind(new_socket, (struct sockaddr *) &source_addr, sizeof(source_addr) );

[MartinMelikMerkumians]

Hi, I am aware of the fact, that you can also fix the source port via bind, and this also works for Linux. I tried this approach, but there is a major problem to this. As you have to bind to INADDR_ANY all or some (depending on the implementation) of the received UDP packets from one or several originators will be delivered to this socket. In OpENer this would mean, that the receiving I/O sockets will not get any consuming I/O messages, resulting in errors like time-outed connection although the originator is sending and so forth.

The best solution without rewriting large portions of OpENer was the use of raw sockets, which I am aware if poses several hardships, like running it as a privileged process. If there is a better solution to this problem I am open for it.

See #152 as reference

[MartinMelikMerkumians]

A possible partial fix for Windows could be the use of the WinPcap library which provides raw sockets. At least WireShark is not asking for admin priviliges at startup

[PeterChristen577]

Hi I am working on a port to STM32 family (ARM Cortex) with LwIP and FreeRTOS. The last weeks i strugled over this raw socket issue to get the implicit messaging working. Enabling raw sockets in lwIP makes it work for OpENer, but the imcoming implicit messages are crippled in the IP stack. Now it is working but finally I had to modify the lwIP stack. For me this is not a good approach. Therefore I would provide support to get off using raw sockets. This would also solve the problems for the Windows ports.

[MartinMelikMerkumians]

Hi @PeterChristen577 I am looking forward to it. If you need help feel free to contact me.

[PeterChristen577]

Hi Martin I took the current master branch to my Linux system and reverted back the changes done for this issue #157. Running this it resulted in a timed out session, the same you have been faced. The I tried to bind the socket to the originators address and not to INADDR_ANY. struct sockaddr_in source_addr = { .sin_addr = {peer_address.sin_addr.s_addr}, .sin_family = AF_INET, .sin_port = htons(2222) }; It works. Maybe I am not respecting possible drawbacks. If this is the case, please lets have a discussion about possible strategies to continue.

[MartinMelikMerkumians]

Can you provide the changes as a patch, so that I can inspect and test them? I thought I tried this also and I had a problem with the conformance tests still.

[PeterChristen577]

Here is the patch: opener_wo_raw.zip

[DevRga]

@PeterChristen577 Thanks for this patch. I'm not able to audit it (my network programming skills are not good enough and I have not done intensive or conformance testing), but so far it seems working on my windows 10 system with implicit messaging. I'll come back if anything goes wrong. @CapXilinx Thanks for all the job on the lib ;)

[MartinMelikMerkumians]

@PeterChristen577 unfortunately your fix most likely won't pass the conformance test. The problem is still with the sending UDP packages, as the CIP protocol can choose from which port Class 1 messages shall be sent. If the sending port is not bound, the system will choose the sending UDP port. e.g., in my test run, the originator demanded the messages to be sent from port 2222, but as the port is not bound to a socket, it is sent via 39219.

If you have an idea for this problem, I am open to try and add it to the code.

[DevRga]

Hi everybody, I've just check on my code. With the fix from @PeterChristen577, the source port is indeed not the 2222, but I can't understand why you try to bind the socket to the peer address : struct sockaddr_in source_addr = { //~ .sin_addr = {INADDR_ANY}, .sin_addr = {peer_address.sin_addr.s_addr}, .sin_family = AF_INET, .sin_port = htons(2222) }; In my case (on WIndows), it seems doing the right way by replacing the addr to socket_data->sin_addr. I can't say if this can have side effects or decrease compatibility... Hope this helps.

[MartinMelikMerkumians]

@DevRga I'll give that a try. As said I thought I tried that too, but the original issue is almost 3 years past, so not sure if I tried that or if it didn't work on all platforms back then

[PeterChristen577]

@CapXilinx : Sorry I did only test with Ethernet Explorer and the Implicit Messaging locked good. But having a look using WireShark I saw well that the source port was not 2222. Now I have a new proposal that looks quite promising:

` struct sockaddr_in source_addr;

memset(&source_addr, 0, sizeof(source_addr));
source_addr.sin_family = AF_INET;
source_addr.sin_port = htons(2222);
source_addr.sin_addr.s_addr = INADDR_NONE;
if(bind(new_socket, (struct sockaddr*) &source_addr, sizeof(source_addr)) < 0) ....

`

By adding a check for the error of these bind I have seen that it could not bind the the originator address. But using INADDR_NONE instead of INADDR_ANY it seems to work, both ports are 2222:

Here is the patch: opener_wo_raw_02.zip

@DevRga Please test it also with Windows ;)

[DevRga]

Hi, With your latest patch, on Windows, with the default sample app, a Forward Open leads to an error message. The bind is not successful, error code 10049 ("The requested address is not valid in its context."). But as the opposite, when I change back the address to INADDR_ANY, it seems doing fine (tested with EnipExplorer, port verified with Wireshark). (this is why it worked with the input socket_data address, set to 0x00000000 (=INADDR_ANY) in my case). So the behaviors are different platform to platform...

[PeterChristen577]

Hi, Yes the use of INADDR_NONE is definitely a dead end. Also the lwIP stack throws an error when the UPD message should be sent. Using INADDR_ANY the produced messages are sent with the correct ports 2222 but the messaging times out after 10 seconds because the the messages coming in are not processed in the just way. Maybe the reason could be that 2 UDP sockets are used and the consumed messages coming in are captured by the wrong one. Bud this is an assumption only. @DevRga : A short question: Is the implicit messaging with Windows working longer than for 10 seconds?

[MartinMelikMerkumians]

@PeterChristen577 yes, that's exactly the reason. If multiple sockets with the same address are opened, the kernel decides to which socket a received message is delivered. A remedy for that would be to program a socket manager, tracking all sockets/addresses opened and reusing the same socket for multiple connections. Received messages must be then manually multiplexed to the correct connections.

This is probably the only solution for the problem, which would require a lot of changes to enable this.

[DevRga]

@PeterChristen577 :In my case, it seems working well (implicit messaging seems working both ways, no packet loss -i guess-, hence no timeout), but as I understand, there is no guarantee that the incoming messages are processed by the consuming socket and not by the producing one (since we have bound to the same port).

[PeterChristen577]

@CapXilinx I did some basic research on UDP socket programming and wrote a very small application creating two UDP sockets using ports 2222, the same as we have in OpENer. I figured out that in this case of two sockets having same address the one which has been created later is the receiving one (with the higher fd number), as a check with ss shows:

State  Recv-Q Send-Q Local Address:Port   Peer Address:Port
UNCONN 3072   0            0.0.0.0:2222        0.0.0.0:*       users:(("udpServer03",pid=1350,fd=4)) 
UNCONN 0      0            0.0.0.0:2222        0.0.0.0:*       users:(("udpServer03",pid=1350,fd=3)) 

Now I exchanged in OpenCommunicationChannels (cipioconnection.c,) the order of the calls from:

OpenConsumingPointToPointConnection
....
OpenProducingPointToPointConnection

to

OpenProducingPointToPointConnection
....
OpenConsumingPointToPointConnection

This works in Linux and with lwIP. But clear for me this is a not safe solution, as it depends from the behaviour of the OS. I did some research how UDP Servers (what this in fact is) should be designed. The common sense is to have only one UDP socket as single point of entry and then to multiplex the messages to the connections as @CapXilinx has proposed few days ago.

[PeterChristen577]

I am back and have rewritten it so that all implicit messages are handled in one and the same UDP socket. From my point of view it works as expected without using any raw sockets. A short brief of the changes:

• Added a global UDP socket for io messaging to g_network_status
• CreateUdpSocket is called once in OpenCommunicationChannels
• CloseUdpSocket is called once in CloseCommunicationChannelsAndRemoveFromActiveConnectionsList
• CreateUdpSocket adapted parameters removed, open the global socket, doing a bind(2) to define the src port
• Remove all socket configuring stuff from all functions called in EstablishIoConnection
• New functions created for additional TTL and Multicast settings formerly done in CreateUdpSocket
• Renamed CheckAndHandleConsumingUdpSockets to CheckAndHandleConsumingUdpSocket

I also consider to replace the member .socket[2] by two boolean variables that indicate if a connection is consuming or producing as it looks like that the socket handles need not to be stored in the connection object anymore. Is this assumption correct?

Should I provide a patch or better a pull request?

Regards Peter

[MartinMelikMerkumians]

Cool, thanks @PeterChristen577 If possible I would prefer a pull request.

Best regards Martin

[PeterChristen577]

@CapXilinx : OK, Pull request Producing connections without raw sockets #346 is there. Can we have a short discussion about the CipConnectionObject.socket[2]? In the new implementation there is no need to store socket handles in the ConnectionObjects even for implicit messaging. In all existing references (except in CloseConnection in cipconnectionmanager.c) they are used to check if a connection is consuming or producing. This could be done easier with two booleans, one for each direction. Or is there a need to keep .socket[2] for explicit messaging I did not see?

Best regards Peter

[PeterChristen577]

@CapXilinx : Unfortunately Till now I did not get any response the my open questions regarding this issue and the related Pull request #346.

[MartinMelikMerkumians]

I am sorry @PeterChristen577 I was on vacation and immediately after in sick leave. Explicit messaging needs only one socket, as it is always a TCP connection in the current implementation (there is a new feature specified which also allows explicit messaging via UDP).

I will inspect your PR asap. Sorry for the delay

[MartinMelikMerkumians]

Finally your PR got merged, so I close this issue now