Windows (MinGW) failures/deprecations and general topics

Broekman commented 3 years ago

Hello,

First of all many thanks for the efforts on EIPScanner, with some hacks I managed to get all features working. These "hacks" were mostly needed on Windows (MinGW) due to some inconsistencies/use of deprecated functions. I didn't test on Linux but I don't believe we have the same issues there as the used socket functionality returns as expected.

I'm currently doing some work on replacing these hacks for some actual implementations that can be pushed back to the library but before I do so I would like some more opinions about the topics at hand (I will make specific tickets where needed later).

errno not set

errno is not set (always zero), resulting in undefined behavior as the result of this is used throughout the library. WSAGetLastError() should be used instead and use the respective WSA error codes (e.g. WSAEAGAIN).

Non-blocking UDP socket returns ETIMEDOUT on recv instead on EAGAIN

E.g. the discovery scanner depends on a return of EAGAIN from the receive function causing it to fail. I would expect the same Linux behavior (https://man7.org/linux/man-pages/man3/recvfrom.3p.html) on Windows but the documentation is limited on this (https://docs.microsoft.com/en-us/windows/win32/api/winsock/nf-winsock-recvfrom).

inet_addr(...) is deprecated after Windows XP

inet_addr(...) is deprecated after Windows XP and actually failing on later versions (i.e. returns INADDR_NONE with the empty string passed in e.g. discovery manager, where defined behavior should be INADDR_ANY. Replace with inet_pton(...) on Windows and optionally keep inet_addr in there with some Windows <= XP version check for backwards compatibility.

E.g. this monstrosity in Endpoint.cpp:

#if defined(__linux__) || defined(__APPLE__)
    if (inet_aton(_host.c_str(), &_addr.sin_addr) < 0) {
#elif defined _WIN32
  #if (_WIN32_WINNT >= 0x0600) //Windows Vista and later
    if (inet_pton(AF_INET, _host.c_str(), &_addr.sin_addr.s_addr) < 0) {
  #else //Windows XP and before (deprecated)
    if ((_addr.sin_addr.s_addr = inet_addr(_host.c_str())) == INADDR_NONE) {
  #endif
#endif
      throw std::system_error(errno, std::generic_category());
    }

Other non-Windows/MinGW related things

Empty (0.0.0.0) socket address info T -> O included in forward open request With the 0.0.0.0 socket address information command specific data included, my EIP devices attempt to send back to 0.0.0.0. I'm pretty sure the address info section should A) be removed or B) already set to the local endpoint. I believe A is according to specifications, it should not be in the FW open request at all. I will check the specs later to be sure but implementing either A) or B) works for me

jadamroth commented 3 years ago

Hello Stefan,

I'll start by saying I'm not as familiar with the windows/mingw functionalities, as we originally developed this library to run solely on linux. The windows implementation was a community contribution from @luntik2012, which we're thankful for.

I'm not able to personally test the windows version, but I'll allow for flexibility in pull requests pending 1) no breaking api changes 2) all tests still pass 3) everything still works on linux

Going through your sections one by one (from my linux perspective):

errno not set I see what you mean. Are you receiving runtime errors/does your program crash, or does this just return a 0 (non-error) value? Nevertheless, I'll create a separate issue

Non-blocking UDP socket returns ETIMEDOUT on recv instead on EAGAIN I can't confirm we've received these errors, but I'll try to replicate your error if you explain a bit more

inet_addr(...) is deprecated after Windows XP Not pretty, but I'll accept this change if it's what you need. Out of curiosity, are you testing with a machine windows xp or earlier? Haven't seen one of those in a while :)

Empty (0.0.0.0) socket address info T -> O included in forward open request Are you running point-to-point or multicast/broadcast? I mostly work with explicit messaging, but I can try to help as much as possible. If you manage to solve the issue by modifying the code, please let me know, so I can see the differences. Also, please attach any specs or docs mentioning this if you find them.

Thanks for using the library, and let me know what else you need

Broekman commented 3 years ago

Hi Adam,

Thanks for your reply, I have seen pretty much every bit of code in the library now and have all components (i.e. implicit, explicit, discovery, file transfers) working properly under Windows 10 (MinGW-W64). At the moment I only use point-to-point. You can have a look at my forked repository for the changes. To summarize:

For socket operations, now using WSAGetLastError() under Windows and as before errno for Unix. Also the respective WSA error codes are used. errno can still be used on Windows for e.g. file I/O however specifically Winsock does not work with errno and had to be replaced (also see: https://docs.microsoft.com/en-us/windows/win32/winsock/error-codes-errno-h-errno-and-wsagetlasterror-2 - specifically: "Error codes set by Windows Sockets are not made available through the errno variable.").
In discovery manager, now using WSAETIMEDOUT/ETIMEDOUT instead of EAGAIN for Windows. (DiscoveryManager.cpp; line 88 in the fork). On Unix systems, EAGAIN is given when the socket tries to read and there is nothing available. Under Winsock, EAGAIN does not exist and exits on the configured/set timeout with WSATIMEDOUT.
Removed the 0.0.0.0 network info additional packet item from the forward open request. Browsed through the specification (CIP vol1_3.24) and couldn't find any reference that this should be added. I did find this in some paper (the bold/italic text) - it is still as expected in the response:

The basic structure of each one of these items is reflected in table 3. The Type ID field shows the type of item encapsulated. For the present project, the four different types of items found in the Wireshark traces are Null Address Item, Unconnected Data Item, Socket Address Info T -> O, and Socket Address Info O -> T. The last one, as will be explained afterwards, is found in the CM response packet, but not in the request.
Replaced init_addr(...) with inet_pton(...) for Windows - note that I decided to completely removed init_addr(...) as this one has been deprecated since Windows XP (i.e. time to upgrade your system).
Resolved some warnings, I usually compile with -Wall -Wextra -Wpedantic -Wconversion. There were a few Wreorder, Wunused, and Wunitialized warnings. There are still some (un)signed comparison, conversion and dereferencing warnings but I left them untouched for now.
Added additional common services to the list.
Added default port (2222 implicit and 44818 explicit) specifiers.

Open topics

In the system error exception throws, std::generic_category() can not handle the WSA errors. Although the runtime behavior is correct, if it throws (as expected), the message is always unknown. I have to see how I can nicely update this and use e.g. the LPTSTR Error buffer under Windows (i.e. no problem for now, just unknown messages).
First forward open, everything is fine and the connection is maintained properly. When a second forward open is done within the same application lifetime (after a normal forward close), everything seems OK however after the normal timeout (multiplier*TO), EIP scanner removes it from the client/open connnection map (causing ...hasOpenConnections to return). I have to analyze a bit more if this is just due to an error in my own part or in the library. _EDIT: already resolved this one, had to do with the fact that close(...) was used instead of closesocket(...) on Windows (all other socket functions have to same name as on Unix, except for this one. MinGW just had a deprecated name macro for close(...). Defining NOOLDNAMES exposes this.

Other than that haven't had any problems yet, all seems to be working nicely! After I have done some more testing and cleared the open topics, I will create a pull-request (or split it into multiple tickets, whatever you would like to see).

Broekman commented 3 years ago

Already resolved open topic point 2: Had to do with the fact that close(...) was used instead of closesocket(...) on Windows (all other socket functions have to same interface as on Unix, except for this one. MinGW just had a deprecated name macro for close(...). Defining NO_OLDNAMES exposes this.

#ifndef NO_OLDNAMES
  int __cdecl access(const char *_Filename,int _AccessMode) __MINGW_ATTRIB_DEPRECATED_MSVC2005;
  int __cdecl chmod(const char *_Filename,int _AccessMode) __MINGW_ATTRIB_DEPRECATED_MSVC2005;
  int __cdecl chsize(int _FileHandle,long _Size) __MINGW_ATTRIB_DEPRECATED_MSVC2005;
  int __cdecl close(int _FileHandle) __MINGW_ATTRIB_DEPRECATED_MSVC2005;
  int __cdecl creat(const char *_Filename,int _PermissionMode) __MINGW_ATTRIB_DEPRECATED_MSVC2005;
  int __cdecl dup(int _FileHandle) __MINGW_ATTRIB_DEPRECATED_MSVC2005;
  int __cdecl dup2(int _FileHandleSrc,int _FileHandleDst) __MINGW_ATTRIB_DEPRECATED_MSVC2005;
  int __cdecl eof(int _FileHandle) __MINGW_ATTRIB_DEPRECATED_MSVC2005;
  long __cdecl filelength(int _FileHandle) __MINGW_ATTRIB_DEPRECATED_MSVC2005;
  int __cdecl isatty(int _FileHandle) __MINGW_ATTRIB_DEPRECATED_MSVC2005;
  int __cdecl locking(int _FileHandle,int _LockMode,long _NumOfBytes) __MINGW_ATTRIB_DEPRECATED_MSVC2005;
  long __cdecl lseek(int _FileHandle,long _Offset,int _Origin) __MINGW_ATTRIB_DEPRECATED_MSVC2005;
  char *__cdecl mktemp(char *_TemplateName)  __MINGW_ATTRIB_DEPRECATED_MSVC2005;
  int __cdecl open(const char *_Filename,int _OpenFlag,...)  __MINGW_ATTRIB_DEPRECATED_MSVC2005;
  int __cdecl read(int _FileHandle,void *_DstBuf,unsigned int _MaxCharCount)  __MINGW_ATTRIB_DEPRECATED_MSVC2005;
  int __cdecl setmode(int _FileHandle,int _Mode) __MINGW_ATTRIB_DEPRECATED_MSVC2005;
  int __cdecl sopen(const char *_Filename,int _OpenFlag,int _ShareFlag,...) __MINGW_ATTRIB_DEPRECATED_MSVC2005;
  long __cdecl tell(int _FileHandle) __MINGW_ATTRIB_DEPRECATED_MSVC2005;
  int __cdecl umask(int _Mode) __MINGW_ATTRIB_DEPRECATED_MSVC2005;
  int __cdecl write(int _Filehandle,const void *_Buf,unsigned int _MaxCharCount) __MINGW_ATTRIB_DEPRECATED_MSVC2005;
#endif

jadamroth commented 3 years ago

Great, I'll review your forked branch and look forward to the pull request (you can submit as one pull request).

Broekman commented 3 years ago

Added a win32 specific error_category definition to get the specific Win32 error descriptions within the exceptions (std::system_error) - just pushed back. This also ticks off the first/last open point. Also cleaned up a bit and instead of the macro's, I now use class functions. The added platform specific stuff is still quite clean but if you have any ideas on how to do it nicer, let me know.

I will create a PR (probably) tomorrow and we can discuss there for the specific points if we want any changes.

Broekman commented 3 years ago

Resolved in PR: https://github.com/nimbuscontrols/EIPScanner/pull/47

nimbuscontrols / EIPScanner