On Wed, Mar 07, 2012 at 11:08:52AM +0900, Isaku Yamahata wrote: > On Tue, Mar 06, 2012 at 08:42:16AM +0900, FUJITA Tomonori wrote: > > On Mon, 5 Mar 2012 09:34:06 +0700 > > Simon Horman <[email protected]> wrote: > > > > > OFPAction.parser() should call the parser of one of the classes registered > > > in _ACTION_TYPES() rather than calling itself until a buffer underflow > > > occurs. > > > > > > Also, it is not necessary to increment offset as the parser method of the > > > registered classes re-parse the type and length and thus do not expect it > > > to be incremented. > > > > > > Signed-off-by: Simon Horman <[email protected]> > > > > > > --- > > > > > > I am unsure of the merit of using assert on error in this code. > > Yeah, a malicious switch can cause this assertion. It's pointless. We > > should handle about this error properly. At lease, please put a > > "FIXME" comment. Assertion is only useful to find our bugs not > > other components' bugs (switches, etc). > > I have to admit some assert I put are pointless. Needs clean up. > OFPParserError (or something like that) needs to be introduced instead of > assert. > > As for malicious packet, the exception is raised up to gevent StreamServer. > And then the connection is disconnected. This is intentional. > (It should be explicitly commented in the code. Okay I'll send a patch) > > If we'd like to be a bit more smarter like skip to the next message, > it can be easily done by catching exception explicitly.
Personally I think it would be nice to catch the exception. The following patch is an attempt to do that. But perhaps the behaviour of closing the connection is better than simply ignoring the packet as this patch does. Avoid loop in OFPAction.parser() OFPAction.parser() should call the parser of one of the classes registered in _ACTION_TYPES() rather than calling itself until a buffer underflow occurs. Also, it is not necessary to increment offset as the parser method of the registered classes re-parse the type and length and thus do not expect it to be incremented. Signed-off-by: Simon Horman <[email protected]> --- v2 * Raise exception rather than asserting that cls_ is not None in OFPAction.parser(). --- ryu/controller/controller.py | 13 +++++++++---- ryu/ofproto/ofproto_v1_0_parser.py | 7 +++++-- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/ryu/controller/controller.py b/ryu/controller/controller.py index 886ee31..0ab3a5f 100644 --- a/ryu/controller/controller.py +++ b/ryu/controller/controller.py @@ -21,6 +21,8 @@ import random from gevent.server import StreamServer from gevent.queue import Queue +from ryu import exception + from ryu.ofproto import ofproto from ryu.ofproto import ofproto_parser from ryu.ofproto import ofproto_v1_0 @@ -127,10 +129,13 @@ class Datapath(object): if len(buf) < required_len: break - msg = ofproto_parser.msg(self, - version, msg_type, msg_len, xid, buf) - #LOG.debug('queue msg %s cls %s', msg, msg.__class__) - self.ev_q.queue(ofp_event.ofp_msg_to_ev(msg)) + try: + msg = ofproto_parser.msg(self, version, msg_type, + msg_len, xid, buf) + #LOG.debug('queue msg %s cls %s', msg, msg.__class__) + self.ev_q.queue(ofp_event.ofp_msg_to_ev(msg)) + except exception.OFPMalformedMessage: + LOG.warn('Received malformed message') buf = buf[required_len:] required_len = ofproto.OFP_HEADER_SIZE diff --git a/ryu/ofproto/ofproto_v1_0_parser.py b/ryu/ofproto/ofproto_v1_0_parser.py index 5ee4e3d..8d5990d 100644 --- a/ryu/ofproto/ofproto_v1_0_parser.py +++ b/ryu/ofproto/ofproto_v1_0_parser.py @@ -17,6 +17,7 @@ import collections import struct from ofproto_parser import MsgBase, msg_pack_into, msg_str_attr +from ryu import exception from ryu.lib import mac from . import ofproto_parser from . import ofproto_v1_0 @@ -144,8 +145,10 @@ class OFPAction(OFPActionHeader): def parser(cls, buf, offset): type_, len_ = struct.unpack_from( ofproto_v1_0.OFP_ACTION_HEADER_PACK_STR, buf, offset) - offset += ofproto_v1_0.OFP_ACTION_HEADER_SIZE - return cls.parser(buf, offset) + cls_ = cls._ACTION_TYPES.get(type_) + if cls_ is None: + raise exception.OFPMalformedMessage + return cls_.parser(buf, offset) @OFPAction.register_action_type(ofproto_v1_0.OFPAT_OUTPUT, -- 1.7.6.3 ------------------------------------------------------------------------------ Virtualization & Cloud Management Using Capacity Planning Cloud computing makes use of virtualization - but cloud computing also focuses on allowing computing to be delivered as a service. http://www.accelacomm.com/jaw/sfnl/114/51521223/ _______________________________________________ Ryu-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/ryu-devel
