DHCP Fails, prints f and returns a value that causes exception

[FoamyGuy]

Using this library on my home network, I am able to successfully connect once and sometimes even a few times re-running the same script. But after a few times it gets into a state where it will consistently fail every time it runs with this exception:

Traceback (most recent call last):
  File "code.py", line 23, in <module>
  File "adafruit_wiznet5k/adafruit_wiznet5k.py", line 202, in __init__
  File "adafruit_wiznet5k/adafruit_wiznet5k.py", line 222, in set_dhcp
  File "adafruit_wiznet5k/adafruit_wiznet5k_dhcp.py", line 494, in request_dhcp_lease
  File "adafruit_wiznet5k/adafruit_wiznet5k_dhcp.py", line 430, in _dhcp_state_machine
TypeError: object with buffer protocol required

Digging in a bit I've found that the crux of the issue comes from here. On my network after a DHCP lease has been established once it seems this starts returning values that cause this condition to execute:

Adafruit_CircuitPython_Wiznet5k/adafruit_wiznet5k/adafruit_wiznet5k_dhcp.py

Lines 257 to 260 in 04e2e21

	xid = _BUFF[4:8]
	if bytes(xid) < self._initial_xid:
	print("f")
	return 0, 0

This prints "f" and then returns 0,0 which cause the raised exception when one of the values is attempted to be used with int.from_bytes

I'm trying to understand what is the actual cause of this exception, and hoping that we can come up with something more helpful to print out than just "f" so that users can easier debug whatever the cause is.

Are there any cases where this condition can be True but aren't fatal errors? If it is always a fatal error we may as well raise it from here instead of printing "f" and returning if there are no cases that will continue anyway.

In troubleshooting I've found that if I simply comment out the return:

        if bytes(xid) < self._initial_xid:
            print("f")
            #return 0, 0

The code can run and successfully fetch data from online.

I don't really know anything about the specifics of DHCP so I'm unsure if this is expected or not. In my case though it seems this state is not actually fatal except for the fact that it's returning the bogus 0s. Commenting that out and allowing it to continue results in success.

[BiffoBear]

• self._transaction_id is assigned a random 32 bit int in DHCP.__init__()
• self._initial_xid is set to self._transaction_id each time send_dhcp_message() is called. This is used to check that the DHCP server response being parsed matches the request sent.
• xid is the transaction ID for the response being parsed in parse_dhcp_response()
• _dhcp_state_machine() increments self.transaction_id then performs an & 0x7FFFFFFF

It seems that if bytes(xid) < self._initial_xid: is a quick sanity check to stop parsing if the response does not match the original request. I'm not clear why the comparison is < and not !=, perhaps because the state machine is incrementing the transaction ID.

There are two reasons that I can see for this test returning 0, 0. First, the DHCP server broadcasts a response to another client's request while our client is waiting for its response. Second, if the comparison is '<' because the state machine increments self._transaction_id then eventually self._transaction_id will wrap around to zero.

All three tests in parse_dhcp_response() return 0, 0 to indicate an unexpected DHCP server response. So all these responses will raise the same exception that you are seeing from the bad xid.

parse_dhcp_response() is called from lines 429 and 454 inside the DHCP state machine. My understanding of this code is that it should not cause a fatal error. The state machine should continue to run. So I suggest wrapping the calls in lines 429 and 454 with try… except blocks and print debugging messages if debugging is active then change all the return 0, 0 statements to raise exceptions.

I suggest that the assert in line 277 is changed to raise for consistency. I don't think a bad server packet should be fatal either.

If this makes sense to you, I'm happy to make the changes, run some tests and issue a PR. I've been hacking away at this package for a while and I feel like I know my way around it.

[BiffoBear]

There are two reasons that I can see for this test returning 0, 0. First, the DHCP server broadcasts a response to another client's request while our client is waiting for its response. Second, if the comparison is '<' because the state machine increments self._transaction_id then eventually self._transaction_id will wrap around to zero.

Wrapping around from 0x7fffffff to 0x00000000 doesn't raise that exception.