Infinite loop in DhcpClass::parseDHCPResponse if more then 1 response packet arrives.

[GoogleCodeExporter]

I know you perhaps should not get a response from more than 1 machine on the 
same network but if you do, and it's more common than one might think of, the 
code will end up in a never ending loop.

I have downloaded Arduino-1.0 (the proposed release that were announced on the 
dev mailing list, later than rc2) and have some issues.
How I detected this was by trying to use the DhcpChatClient example.
In my network I will get (working on removing but still) 2 replies to a DHCP 
request.
I think this messes up the EthernetUDP / DHCP implementation.

This is somewhat related to issue #669 as well and it doesn't work even if I 
implement the proposed changes there.
This specific issue is about DHCP but I think it's an issue that might turn up 
elsewhere as well.

In the beginning of DhcpClass::parseDHCPResponse there is a 
_dhcpUdpSocekt.parsePacket() which updates the private EthernetUDP._remaining 
counter. So far everything is well and the packet starts to be parsed.

A while loop begins to parse the packet until .available() is 0 on the udp 
socket.
While parsing, another packet arrives to the buffer in the w5100.
This will add to the count that .available() returns which is correct but I 
will not be allowed to read them after the _remaining counter in EthernetUDP 
goes down to 0.
This will make the while(_dhcpUdpSocket.available()) run forever since there is 
correctly bytes available but I'm not allowed to read them, unless I would do 
another .parsePacket() on the UDP socket since all read methods look at the 
_remaining counter which is already 0.

You simply can not trust to .read() the socket until .available() turns 0 since 
you can not be sure that the _remaining counter and the buffer in w5100 match.

My HACKY workaround was to make EthernetUDP._remaining public instead of 
private and do 
while(_dhcpUdpSocket._remaining>0) 
instead of 
while(_dhcpUdpSocket.available())
and simply throw away the second response.
This is NOT the correct way to solve and I'm not good enough at c/c++ to do a 
correct solution but it shows the issue

Anything BUT ending up in a never ending loop would be fine, I could live with 
not getting an IP even though the first response would give me one. The best 
would be do deal with the situation in a proper way and use the first valid 
response (or perhaps only the first response, period) and ignore everything 
else.

Original issue reported on code.google.com by pe...@birchroad.net on 2 Dec 2011 at 3:31

[GoogleCodeExporter]

Sorry, my fix only works when changes from #669 is applied....

Original comment by pe...@birchroad.net on 2 Dec 2011 at 3:43

[GoogleCodeExporter]

I've only taken a very brief look at the Dhcp.cpp code, but I wonder if 
changing EthernetUDP.available() to return _remaining rather than 
W5100.getRXReceivedSize(_sock) makes more sense. The logic being that no bytes 
should be available until parsePacket() has been called, and then, only the 
bytes that correspond to the current packet. There are a couple references to 
available() within EthernetUDP but they could be updated to refer directly to 
W5100.getRXReceivedSize(_sock).

I'll try and make the changes and upload an updated EthernetUDP.cpp later.

Original comment by dy...@deedums.com on 3 Dec 2011 at 5:14

[GoogleCodeExporter]

I've attached an updated version of EthernetUdp.cpp to issue 669 which I hope 
will fix this for you. Can you please test and let me know if it's fixed it for 
you?

Original comment by dy...@deedums.com on 3 Dec 2011 at 7:43

[GoogleCodeExporter]

It works perfectly now, thanks.
When can we expect to see 669 committed to github?

Original comment by pe...@birchroad.net on 5 Dec 2011 at 7:40

[GoogleCodeExporter]

I'm not sure. I submitted the bug and patch a couple months back but I'm not 
sure of the right way to bring it to the attention of the Arduino folks. 

Original comment by dy...@deedums.com on 5 Dec 2011 at 10:38