Crash when OFPT_ERROR comes with incomplete data

When an OFPT_FLOW_MOD is rejected through an OFPT_ERROR, some vendors add the OFPT_FLOW_MOD message as part of the data field in the OFPT_ERROR. Some vendors don't send anything and Brocade does a mix: send only part of the message. The python-openflow tries to extract the full flow-mod but crases because buffer is not big enough.

Error: FlowMod.buffer_id; unpack_from requires a buffer of at least 4 bytes; fmt = !I, buff = b'\x008 \xfe\x00\x03\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x80\x00', offset = 56.

Libpcap provided in the link [1]. It would be good that, in situations such as this one, the python-openflow just ignored the data (or extract only the first 8 bytes (header)).

[1] https://dl.dropboxusercontent.com/u/38035109/of10-3.pcap

[diraol]

Hi @amlight thanks for the feedback.

Firstly, I've uploaded the pcap file (zipped) here for tracking/history purposes.
of10-3.zip

[diraol]

@renanrodrigo please, take a look at it.

While dealing with error messages, if we can't unpack the message that generated the error we may get the unpack exception an deal with it.
We have two choices here.

1. leave the binary data on the field
2. leave just an unpacked header from that binary data if there is one.

[renanrodrigo]

The OpenFlow 1.0 specification says switches must send back at least 64 bytes of the bad request with the error packet, so we can expect that many messages won't be unpacked correctly.
More than that, the switch must send the ofpt_error_msg with the same xid as the message which generated the error.
So, IMHO, one can identify which message generated the error only by the xid of the Error message header and check what did happen according to type and code.
I think the best approach is option 1: leaving the binary data sent by the switch on the field bypasses any unpack error while keeping the bytes there to further analysis when needed.

I think we should try to unpack the message if possible. The XID helps, it is how we have being doing all this time at AmLight, but having the source message unpacked helps a lot. Sometimes we install dozen of flows in parallel. My suggestion:

  if len(msg.data) > 8: 
     header = unpack(msg.data[:8]) 

  if len(msg.data) == header.length:
     msg.data = unpack(msg.data)
  else:
     pass
except:
   pass

[renanrodrigo]

@amlight Roger that; working on it.

[cemsbr]

A possible misled interpretation is that the message sent to the switch was only the header. Besides, we would lose information past 8 bytes that cannot be completely unpacked.

[beraldoleal]

@renanrodrigo stand in position sr.

@amlight I agree with @cemsbr, we discussed and we think that saving only partial data is inconsistent. Packing and Unpacking an OpenFlow Object must be bi-directional, and your proposal breaks this.

If we add a method, let's say for instance message_from_data() or header_from_data() that returns whatever it is possible to unpack, it works for you?

Also, I think that this issue title is strange. So far we remember, today we don't unpack data. data is unpacked as binary data only. So this crash, do you have a complete output (trace)?