EthernetClient.cpp doesn't close socket when IP Address is 0.0.0.0 or 255.255.255.255

[gtaspider]

When you try to connect to 0.0.0.0 or 255.255.255.255 multiply times, you won't be able to connect to anything anymore after the 4th try (because of the limitation of 4 sockets per w5100)
This is because in EthernetClient.cpp there is at line 57:
if (!::connect(_sock, rawIPAddress(ip), port)) { _sock = MAX_SOCK_NUM; return 0; }

and the connect function in socket.cpp at line 82:
if ( ((addr[0] == 0xFF) && (addr[1] == 0xFF) && (addr[2] == 0xFF) && (addr[3] == 0xFF)) || ((addr[0] == 0x00) && (addr[1] == 0x00) && (addr[2] == 0x00) && (addr[3] == 0x00)) || (port == 0x00) ) return 0;

So, if the IP is 0.0.0.0 or 255.255.255.255 the socket will be opend, the status will be set to SnSR::INIT but the socket will not close after it fails because of the IP so it will be in INIT State forever.

To fix this simply add close(_sock) in the if statement:
if (!::connect(_sock, rawIPAddress(ip), port)) { close(_sock); _sock = MAX_SOCK_NUM; return 0; }

Greetings,
Spider

[Rotzbua]

Both IP addresses have a special meaning and function.
Why you would like to connect to them? What is your suggested use case?

[gtaspider]

I do not want to discuss why someone want to connect to 0.0.0.0, 255.255.255.255 or to port 0. I just want to report this error.

But here is an example case:
A global variable IPAddress IPtoConnectTo;
In the loop the user tries to allocate the IP Address with Ethernet.maintain();
If DHCP succeed, the global variable will be changed to dns ip:
IPtoConnectTo = Ethernet.dnsServerIP();
The user tries at the same time in the loop to connect to the server with
Client..connect(IPtoConnectTo,80);
If DHCP succeed at first time, everything is fine, if the Arduino is plugged to the Ethernet Minutes or Hours later, the Client..connect(IPtoConnectTo,80); will retry 4 times and then will never work again (neither will DHCP because DHCP also tries to create an UDP socket, which fails as all 4 sockets are 'in use')

[Rotzbua]

@gtaspider Sorry for my confusing answer. My intention was to ask for a example to understand the bug you reported.

As I understand the code, you can not connect with int EthernetClient::connect(IPAddress ip, uint16_t port) to 0.0.0.0 and others.

and the connect function in socket.cpp at line 82:
if ( ((addr[0] == 0xFF) && (addr[1] == 0xFF) && (addr[2] == 0xFF) && (addr[3] == 0xFF)) || ((addr[0] == 0x00) && (addr[1] == 0x00) && (addr[2] == 0x00) && (addr[3] == 0x00)) || (port == 0x00) ) return 0;

You mentioned this right. But there is no connection command sent to the w5100. So the socket status should not be changed.

The reason for this bug seems to be this line of code in EthernetClient.cpp:

socket(_sock, SnMR::TCP, _srcport, 0);

Where the socket is initialized.

I would prefer a if-statement before this line of code in EthernetClient.cpp. Simmilar to socket:

if ( ((addr[0] == 0xFF) && (addr[1] == 0xFF) && (addr[2] == 0xFF) && (addr[3] == 0xFF)) ||
((addr[0] == 0x00) && (addr[1] == 0x00) && (addr[2] == 0x00) && (addr[3] == 0x00)) ||
(port == 0x00) )
return 0;

[gtaspider]

@Rotzbua: Unfortunately this won't work as the socket function does not have the IP Address parameter (as the function don't need that) but the port parameter only.
The only thing you could check in the socket function is, that the port != 0, the ip checking has to be done in the connect() function. And if it's fails, the socket has to be closed.

[Rotzbua]

This would be my code: (it would work ;))
ethernetclient.cpp:

int EthernetClient::connect(IPAddress ip, uint16_t port) {
  if (_sock != MAX_SOCK_NUM)
    return 0;

  for (int i = 0; i < MAX_SOCK_NUM; i++) {
    uint8_t s = socketStatus(i);
    if (s == SnSR::CLOSED || s == SnSR::FIN_WAIT || s == SnSR::CLOSE_WAIT) {
      _sock = i;
      break;
    }
  }

  if (_sock == MAX_SOCK_NUM)
    return 0;
  
  // do not initialize socket for IP addresses which are ignored by socket.h
  if ( ((addr[0] == 0xFF) && (addr[1] == 0xFF) && (addr[2] == 0xFF) && (addr[3] == 0xFF)) ||
       ((addr[0] == 0x00) && (addr[1] == 0x00) && (addr[2] == 0x00) && (addr[3] == 0x00)) ||
       (port == 0x00) )
     return 0;

  _srcport++;
  if (_srcport == 0) _srcport = 49152;          //Use IANA recommended ephemeral port range 49152-65535
  socket(_sock, SnMR::TCP, _srcport, 0);

  if (!::connect(_sock, rawIPAddress(ip), port)) {
    _sock = MAX_SOCK_NUM;
    return 0;
  }

  while (status() != SnSR::ESTABLISHED) {
    delay(1);
    if (status() == SnSR::CLOSED) {
      _sock = MAX_SOCK_NUM;
      return 0;
    }
  }

  return 1;
}

In case of a good programming principle "single point of truth", I would agree with your solution. Then there should be a documentation that tells the user what IP addresses are not usable.
Sadly the libraries have a lack of documentation.

[gtaspider]

Ah, sorry I thought you meant the connect function in socket.cpp.
Yes I think your soulution is cleaner, I would recommend to check for the error before the for loop (since the for while isn't neede for the error checking and just costs time in this special case) and addr[] should be replaced with ip[]

int EthernetClient::connect(IPAddress ip, uint16_t port) {
	if (_sock != MAX_SOCK_NUM) {
		return 0;
	}
	// do not initialize socket for IP addresses which are ignored by socket.h
	if (((ip[0] == 0xFF) && (ip[1] == 0xFF) && (ip[2] == 0xFF) && (ip[3] == 0xFF))
			|| ((ip[0] == 0x00) && (ip[1] == 0x00) && (ip[2] == 0x00) && (ip[3] == 0x00))
			|| (port == 0x00))
		return 0;

	for (int i = 0; i < MAX_SOCK_NUM; i++) {
		uint8_t s = socketStatus(i);
		if (s == SnSR::CLOSED || s == SnSR::FIN_WAIT || s == SnSR::CLOSE_WAIT) {
			_sock = i;
			break;
		}
	}

	if (_sock == MAX_SOCK_NUM) {
		return 0;
	}

	_srcport++;
	if (_srcport == 0)
		_srcport = 49152;          //Use IANA recommended ephemeral port range 49152-65535
	socket(_sock, SnMR::TCP, _srcport, 0);

	if (!::connect(_sock, rawIPAddress(ip), port)) {
		_sock = MAX_SOCK_NUM;
		return 0;
	}

	while (status() != SnSR::ESTABLISHED) {
		delay(1);
		if (status() == SnSR::CLOSED) {
			_sock = MAX_SOCK_NUM;
			return 0;
		}
	}

	return 1;
}

[PaulStoffregen]

I believe I have this fix integrated into my Ethernet fork (which will soon become Arduino Ethernet 2.0.0). If anyone cares to take a look, the check for these 2 special cases is here:

https://github.com/PaulStoffregen/Ethernet/blob/master/EthernetClient.cpp#L50

If anyone has a test sketch to run, please let me know? I'd be happy to run the test.

[PaulStoffregen]

Fixed by 2.0.0