read_response() possibly returning less than expected number of bytes

[mct]

Thank you for the NATMPM library! I've been using the get_public_address() function in cronjobs, which has been very convenient. :-)

One issue I noticed is that when my internet connection went down, the cronjob started failing with an uncaught exception in PublicAddressResponse. I suspect what's happening is read_response() is returning some number of bytes greater than 0, but less than 12. PublicAddressResponse assumes exactly 12 bytes will be passed.

Unfortunately, I don't know what read_response() returned. (I wish there was an easy way to have python generate the equivalent of a core file when a program unexpectedly exits, rather than just the traceback, so that variables could be examined postmortem...) But, it's likely I can reproduce the network environment in which this was observed, and examine the value directly. I'll update the bug if I can.

Here's the traceback:

Traceback (most recent call last):
  [..]
  File "/home/mct/.local/lib/python2.7/site-packages/py_natpmp-0.2.2-py2.7.egg/natpmp/NATPMP.py", line 264, in get_public_address
    addr_response = send_request_with_retry(gateway_ip, addr_request, response_data_class=PublicAddressResponse, retry=retry)
  File "/home/mct/.local/lib/python2.7/site-packages/py_natpmp-0.2.2-py2.7.egg/natpmp/NATPMP.py", line 364, in send_request_with_retry
    data = response_data_class(data)
  File "/home/mct/.local/lib/python2.7/site-packages/py_natpmp-0.2.2-py2.7.egg/natpmp/NATPMP.py", line 156, in __init__
    version, opcode, result, sec_since_epoch, self.ip_int = struct.unpack("!BBHII", bytes)
struct.error: unpack requires a string argument of length 12

Thanks! -mct

[yimingliu]

I suspect you're correct. The NATPMP draft standard declares the response header to be of a particular format, which I was unpacking as BBHII, which is byte,byte,unsigned_short,integer,integer, 1+1+2+4+4 = 12 bytes. If this first read doesn't come back with 12 bytes in the header, then the library unfortunately crashes. Which shouldn't happen.

The first part of this is that the library needs to do some more error-checking on the response being returned. The second part is why there is a response of < 12 bytes. This library hasn't been updated since a few drafts ago, while the standard itself has become RFC 6886. I should go back and read the standard again. Also need to make sure that recvfrom() is done before returning from the read call. These are pretty obvious issues in hindsight, but at the time I was most likely in some kind of rush to get back to the main project that I was working on, instead of the support tools here. I'll get on this tonight.

Would definitely appreciate the value that's triggering this bug though. Curious to see if it's a partial value that's in the buffer, or something completely new.

[mct]

I captured the bytestream. I was wrong, it's actually 16 bytes! It appears to be a well formatted response for the first 12 bytes, then has another 4 bytes of zeros tacked on to the end:

Bytes, len 16:  '\x00\x80\x00\x03\xc5\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00'

-mct

[yimingliu]

Thank you for that! So it looks like for this issue this time is just extraneous bytes in the buffer, which we can fix easily by actually passing in the right 12-byte buffer size when sending a public address request (as opposed to the usual 16-byte structures from port mapping requests), and also truncating to the proper length if it's too long. Kind of a quick hack for now; let me know if that fixes your immediate problem.

Will still have to catch undersized responses and deal with that.