Huge thanks to our Platinum Members Endace and LiveAction,
and our Silver Member Veeam, for supporting the Wireshark Foundation and project.

Wireshark-dev: [Wireshark-dev] h.223 robustness fixes

From: Richard van der Hoff <richardv@xxxxxxxxxxxxx>
Date: Tue, 20 Feb 2007 12:11:58 +0000
Hi,

This patch improves the general robustness of the h.223 dissector (making it less likely to crash on malformed data).

Hopefully this also fixes a bug raised by Fabio Sguanci a few weeks ago. Fabio: I think a better way to fix the problem is to stop the dissector crashing when it finds a malformed PDU, so that it just treats the first pdu as malformed; there is then no need to special-case it.

I don't think this patch will apply cleanly without the "bitswapping" patch I submitted yesterday.

Thanks,

Richard
Index: plugins/h223/packet-h223.c
===================================================================
--- plugins/h223/packet-h223.c	(revision 11938)
+++ plugins/h223/packet-h223.c	(working copy)
@@ -47,6 +47,11 @@
 
 #include <string.h>
 
+/* #define DEBUG_H223 */
+
+/* debug the mux-pdu defragmentation code. warning: verbose output! */
+/* #define DEBUG_H223_FRAGMENTATION */
+
 #define PROTO_TAG_H223	"H223"
 
 /* Wireshark ID of the H.223 protocol */
@@ -91,6 +96,7 @@
 static int hf_h223_al1_framed = -1;
 static int hf_h223_al2 = -1;
 static int hf_h223_al2_sequenced = -1;
+static int hf_h223_al2_unsequenced = -1;
 static int hf_h223_al2_seqno = -1;
 static int hf_h223_al2_crc = -1;
 static int hf_h223_al2_crc_bad = -1;
@@ -147,7 +153,8 @@
 	"fragments"
 };
 
-static guint32 pdu_offset; /* offset of the last pdu to start being dissected in the last packet to start being dissected */
+/* this is a fudge to pass pdu_offset into add_h223_mux_element() */
+static guint32 pdu_offset;
 
 /***************************************************************************
  *
@@ -233,16 +240,6 @@
 
 /* we have this information for each stream */
 typedef struct {
-    gboolean current_pdu_header_parsed;
-    
-    guint32 current_pdu_minlen;
-    guint32 current_pdu_read;
-    
-    guint32 header_buf;
-    guint32 tail_buf;
-    
-    gboolean first_pdu;
-    
     h223_mux_element_listitem* mux_table[16];
 } h223_call_direction_data;
 
@@ -309,7 +306,7 @@
     li->me = me;
 }
 
-static h223_mux_element* find_h223_mux_element(h223_call_direction_data* direct, guint8 mc, guint32 framenum)
+static h223_mux_element* find_h223_mux_element(h223_call_direction_data* direct, guint8 mc, guint32 framenum, guint32 pkt_offset)
 {
     h223_mux_element_listitem* li;
     
@@ -319,7 +316,7 @@
     
     while( li && li->next && li->next->first_frame < framenum )
         li = li->next;
-    while( li && li->next && li->next->first_frame == framenum && li->next->pdu_offset < pdu_offset )
+    while( li && li->next && li->next->first_frame == framenum && li->next->pdu_offset < pkt_offset )
         li = li->next;
     if( li ) {
         return li->me;
@@ -370,7 +367,6 @@
 {
     int i;
 	h223_mux_element *mc0_element;
-    direct -> first_pdu = TRUE;
 
     for ( i = 0; i < 16; ++i )
         direct->mux_table[i] = NULL;
@@ -401,6 +397,9 @@
 
     if( subcircuit == NULL ) {
         subcircuit = circuit_new( CT_H223, circuit_id, pinfo->fd->num );
+#ifdef DEBUG_H223
+        g_debug("%d: Created new circuit %d for call %p VC %d", pinfo->fd->num, circuit_id, call_info, vc);
+#endif
         vc_info = h223_vc_info_new( call_info );
         circuit_add_proto_data( subcircuit, proto_h223, vc_info );
     } else {
@@ -486,6 +485,7 @@
     return data;
 }
 
+/* called from the h245 dissector to handle a MultiplexEntrySend message */
 static void h223_set_mc( packet_info* pinfo, guint8 mc, h223_mux_element* me )
 {
     circuit_t *circ = find_circuit( pinfo->ctype, pinfo->circuit_id, pinfo->fd->num );
@@ -499,6 +499,7 @@
     }
 }
 
+/* called from the h245 dissector to handle an OpenLogicalChannelAck message */
 static void h223_add_lc( packet_info* pinfo, guint16 lc, h223_lc_params* params )
 {
     circuit_t *circ = find_circuit( pinfo->ctype, pinfo->circuit_id, pinfo->fd->num );
@@ -540,6 +541,7 @@
     const guint8* data = tvb_get_ptr( tvb, 0, len );
     unsigned char crc = 0;
     guint32 pos = 0;
+    DISSECTOR_ASSERT(tvb_reported_length(tvb) >= 1);
     while ( len-- )
         crc = crctable[crc^data[pos++]];
     return crc;
@@ -555,14 +557,15 @@
     proto_item *al_item;
     proto_tree *al_subtree;
     proto_item *al_subitem = NULL;
+    proto_item *tmp_item;
     tvbuff_t *next_tvb = NULL;
     dissector_handle_t subdissector = lc_params->subdissector;
     guint32 len = tvb_reported_length(tvb);
-    gboolean all_done = FALSE;
 
     guint8 calc_checksum;
     guint8 real_checksum;
-    gboolean al2_sequenced;
+    gboolean al2_sequenced = FALSE;
+    int data_start;
 
     switch( lc_params->al_type ) {
     case al1Framed:
@@ -573,56 +576,61 @@
         if(lc_params->al_type == al1Framed)
             proto_tree_add_boolean_hidden(al_tree, hf_h223_al1_framed, tvb, 0, 1, TRUE );
         next_tvb = tvb;
+        al_subitem = proto_tree_add_item(al_tree, hf_h223_al_payload, next_tvb, 0, -1, FALSE);
         break;
+        
+    case al2WithSequenceNumbers:
+        al2_sequenced = TRUE;
+        /* fall-through */
     case al2WithoutSequenceNumbers:
-    case al2WithSequenceNumbers:
-        if( lc_params->al_type == al2WithoutSequenceNumbers ) {
-            next_tvb = tvb_new_subset( tvb, 0, len-1, len-1 );
-            al2_sequenced = FALSE;
-        } else {
-            next_tvb = tvb_new_subset( tvb, 1, len-2, len-2 );
-            al2_sequenced = TRUE;
-        }
+        tmp_item = proto_tree_add_boolean(vc_tree, hf_h223_al2, tvb, 0, 0, TRUE );
 
-        al_item = proto_tree_add_none_format(vc_tree, hf_h223_al2, tvb, 0, -1, "H223 AL2 (with%s sequence numbers)",
-                al2_sequenced?"":"out" );
+        al_item = proto_tree_add_item(vc_tree,
+                                      al2_sequenced?hf_h223_al2_sequenced:hf_h223_al2_unsequenced,
+                                      tvb, 0, -1,FALSE);
         al_tree = proto_item_add_subtree (al_item, ett_h223_al2);
 
+        PROTO_ITEM_SET_GENERATED(tmp_item);
+
+        /* check minimum payload length */
+        if(len < (al2_sequenced?2:1))
+            THROW(BoundsError);
+        
+        data_start = 0;
         if( al2_sequenced ) {
-            proto_tree_add_boolean_hidden(al_tree, hf_h223_al2_sequenced, tvb, 0, 1, TRUE );
-            proto_tree_add_uint_format(al_tree, hf_h223_al2_seqno, tvb, 0, 1, tvb_get_guint8( tvb, 0 ),
-                "Sequence number: %u", tvb_get_guint8( tvb, 0 ) );
+            proto_tree_add_item(al_tree, hf_h223_al2_seqno, tvb, 0, 1, TRUE);
+            data_start++;
         }
 
+        next_tvb = tvb_new_subset( tvb, data_start, len-1-data_start, len-1-data_start );
+        al_subitem = proto_tree_add_item(al_tree, hf_h223_al_payload, next_tvb, 0, -1, FALSE);
+
         calc_checksum = h223_al2_crc8bit(tvb);
         real_checksum = tvb_get_guint8(tvb, len - 1);
+
         if( calc_checksum == real_checksum ) {
-            al_subitem = proto_tree_add_item(al_tree, hf_h223_al_payload, next_tvb, 0, -1, FALSE);
             proto_tree_add_uint_format(al_tree, hf_h223_al2_crc, tvb, len - 1, 1, real_checksum,
                 "CRC: 0x%02x (correct)", real_checksum );
         } else {
-            call_dissector(data_handle, tvb, pinfo, al_tree);
-            proto_tree_add_boolean_hidden( al_tree, hf_h223_al2_crc_bad, tvb, len - 1, 1, TRUE );
             proto_tree_add_uint_format(al_tree, hf_h223_al2_crc, tvb, len - 1, 1, real_checksum,
                 "CRC: 0x%02x (incorrect, should be 0x%02x)", real_checksum, calc_checksum );
-            all_done = TRUE;
+            tmp_item = proto_tree_add_boolean( al_tree, hf_h223_al2_crc_bad, tvb, len - 1, 1, TRUE );
+            PROTO_ITEM_SET_GENERATED(tmp_item);
+
+            /* don't pass pdus which fail checksums on to the subdissector */
+            subdissector = data_handle;
         }
         break;
     default:
-        break;
+        call_dissector(data_handle, tvb, pinfo, vc_tree);
+        return;
     }
 
     if (!subdissector)
         subdissector = data_handle;
 
-    if(next_tvb && al_tree && !al_subitem && !all_done)
-        al_subitem = proto_tree_add_item(al_tree, hf_h223_al_payload, next_tvb, 0, -1, FALSE);
-
-    if(next_tvb && al_subitem && !all_done) {
-        al_subtree = proto_item_add_subtree(al_subitem, ett_h223_al_payload);
-        call_dissector(subdissector, next_tvb, pinfo, al_subtree);
-    } else if ( !all_done )
-        call_dissector(data_handle, tvb, pinfo, vc_tree);
+    al_subtree = proto_item_add_subtree(al_subitem, ett_h223_al_payload);
+    call_dissector(subdissector, next_tvb, pinfo, al_subtree);
 }
 
 /************************************************************************************
@@ -633,24 +641,22 @@
 
 /* dissect a fragment of a MUX-PDU which belongs to a particular VC
  *
- * tvb	  	buffer containing the whole MUX-PDU
- * offset 	offset within the MUX-PDU of this fragment
+ * tvb	  	buffer containing the MUX-PDU fragment
  * pinfo	info on the packet containing the last fragment of the MUX-PDU
- * pkt_offset	offset within that packet of the start of the final fragment of
- * 		the MUX_PDU
+ * pkt_offset   offset within the block from the superdissector where the
+ *                fragment starts (must increase monotonically for constant pinfo->fd->num)
  * pdu_tree	dissection tree for the PDU; a single item will be added (with
  * 		its own subtree)
  * vc		VC for this SDU
- * frag_len	length of the MUX-SDU fragment
  * end_of_mux_sdu true if this is a segmentable VC and this is the last
  * 		fragment in an SDU
  */
-static void dissect_mux_sdu_fragment(tvbuff_t *tvb, guint32 offset,
+static void dissect_mux_sdu_fragment(tvbuff_t *next_tvb,
                                     packet_info *pinfo,
                                     guint32 pkt_offset,
                                     proto_tree *pdu_tree,
                                     h223_call_info* call_info,
-                                    guint16 vc, gint frag_len, gboolean end_of_mux_sdu)
+                                    guint16 vc, gboolean end_of_mux_sdu)
 {
     /* update the circuit details before passing to a subdissector */
     guint32 orig_circuit = pinfo->circuit_id;
@@ -659,35 +665,29 @@
     pinfo->ctype=CT_H223;
 
     TRY {
-        tvbuff_t *next_tvb = tvb_new_subset(tvb, offset, frag_len, frag_len);
         circuit_t *subcircuit=find_circuit(pinfo->ctype,pinfo->circuit_id,pinfo->fd->num);
-        dissector_handle_t subdissector = NULL;
         proto_tree *vc_tree = NULL;
         proto_item *vc_item;
         h223_vc_info *vc_info = NULL;
         h223_lc_params *lc_params = NULL;
-        gboolean stuffing = ( vc == 0 && frag_len == 0 );
 
-        if(pdu_tree && !stuffing) {
-            vc_item = proto_tree_add_uint(pdu_tree, hf_h223_mux_vc, next_tvb, 0, frag_len, vc);
+        if(pdu_tree) {
+            vc_item = proto_tree_add_uint(pdu_tree, hf_h223_mux_vc, next_tvb, 0, tvb_reported_length(next_tvb), vc);
             vc_tree = proto_item_add_subtree (vc_item, ett_h223_mux_vc);
         }
 
-        if( stuffing ) {
-            next_tvb = NULL;
-            subdissector = data_handle;
-        } else if( subcircuit == NULL ) {
-            g_message( "Frame %d: no subcircuit id %d found for circuit %d id %d, vc %d", pinfo->fd->num,
-                       pinfo->circuit_id, orig_ctype, orig_circuit, vc );
-            subdissector = data_handle;
+        if( subcircuit == NULL ) {
+            g_message( "Frame %d: Subcircuit id %d not found for call %p VC %d", pinfo->fd->num,
+                       pinfo->circuit_id, call_info, vc );
         } else {
             vc_info = circuit_get_proto_data(subcircuit, proto_h223);
             if( vc_info != NULL ) {
                 lc_params = find_h223_lc_params( vc_info, pinfo->p2p_dir, pinfo->fd->num );
             }
-            if( lc_params == NULL ) {
-                subdissector = data_handle;
-            } else {
+        }
+
+        
+        if( lc_params != NULL ) {
                 if( lc_params->segmentable && lc_params->al_type != al1NotFramed ) {
                     stream_t *substream;
                     stream_pdu_fragment_t *frag;
@@ -695,25 +695,37 @@
                     substream = find_stream_circ(subcircuit,pinfo->p2p_dir);
                     if(substream == NULL )
                         substream = stream_new_circ(subcircuit,pinfo->p2p_dir);
-                    frag = stream_find_frag(substream,pinfo->fd->num,offset+pkt_offset);
+                    frag = stream_find_frag(substream,pinfo->fd->num,pkt_offset);
+
                     if(frag == NULL ) {
-                        frag = stream_add_frag(substream,pinfo->fd->num,offset+pkt_offset,
+#ifdef DEBUG_H223
+                        g_debug("%d: New H.223 VC fragment: Parent circuit %d; subcircuit %d; offset %d; len %d, end %d",
+                                pinfo->fd->num, orig_circuit, pinfo->circuit_id, pkt_offset, tvb_reported_length(next_tvb), end_of_mux_sdu);
+#endif
+                        frag = stream_add_frag(substream,pinfo->fd->num,pkt_offset,
                                                next_tvb,pinfo,!end_of_mux_sdu);
+                    } else {
+#ifdef DEBUG_H223
+                        g_debug("%d: Found H.223 VC fragment: Parent circuit %d; subcircuit %d; offset %d; len %d, end %d",
+                                pinfo->fd->num, orig_circuit, pinfo->circuit_id, pkt_offset, tvb_reported_length(next_tvb), end_of_mux_sdu);
+#endif
                     }
+
                     next_tvb = stream_process_reassembled(
                         next_tvb, 0, pinfo, 
                         "Reassembled H.223 AL-PDU",
                         frag, &h223_al_frag_items,
                         NULL, vc_tree);
                 }
-            }
-        }
 
-        if(next_tvb) {
-            if(lc_params)
-                dissect_mux_al_pdu(next_tvb, pinfo, vc_tree,/* subcircuit,*/ lc_params );
-            else
-                call_dissector(subdissector,next_tvb,pinfo,vc_tree);
+                if(next_tvb) {
+                    /* fudge to pass pkt_offset down to add_h223_mux_element,
+                     * should it be called */
+                    pdu_offset = pkt_offset;
+                    dissect_mux_al_pdu(next_tvb, pinfo, vc_tree,/* subcircuit,*/ lc_params );
+                }
+        } else {
+            call_dissector(data_handle,next_tvb,pinfo,vc_tree);
         }
     }
 
@@ -743,7 +755,21 @@
     return length;
 }
 
-static guint32 dissect_mux_payload_by_me_list( tvbuff_t *tvb, packet_info *pinfo, guint32 pkt_offset, proto_tree *pdu_tree,
+/* dissect part of a MUX-PDU payload according to a multiplex list
+ *
+ * tvb		buffer containing entire mux-pdu payload
+ * pinfo	info on the packet containing the last fragment of the MUX-PDU
+ * pkt_offset   offset within the block from the superdissector where the
+ *                MUX-PDU starts (must increase monotonically for constant
+ *                pinfo->fd->num)
+ * pdu_tree	dissection tree for the PDU
+ * call_info	data structure for h223 call
+ * me		top of mux list
+ * offset	offset within tvb to start work
+ * endOfMuxSdu	true if the end-of-sdu flag was set
+ */
+static guint32 dissect_mux_payload_by_me_list( tvbuff_t *tvb, packet_info *pinfo, guint32 pkt_offset,
+                                               proto_tree *pdu_tree,
                                                h223_call_info* call_info, h223_mux_element* me, guint32 offset, gboolean endOfMuxSdu )
 {
     guint32 len = tvb_reported_length(tvb);
@@ -767,22 +793,38 @@
 		frag_len = len - offset;
 	    else
 		frag_len = me->repeat_count;
-	    dissect_mux_sdu_fragment( tvb, offset, pinfo, pkt_offset, pdu_tree,
-	                              call_info, me->vc, frag_len, (offset+frag_len==len) && endOfMuxSdu);
-	    offset += frag_len;
+	    if(frag_len > 0) {
+		tvbuff_t *next_tvb;
+		next_tvb = tvb_new_subset(tvb, offset, frag_len, frag_len);
+		dissect_mux_sdu_fragment( next_tvb, pinfo, pkt_offset + offset, pdu_tree,
+					  call_info, me->vc, (offset+frag_len==len) && endOfMuxSdu);
+		offset += frag_len;
+	    }
 	}
 	me = me->next;
     }
     return offset;
 }
 
-/* dissect the payload of a MUX-PDU */
-static void dissect_mux_payload( tvbuff_t *tvb, packet_info *pinfo, guint32 pkt_offset, proto_tree *pdu_tree,
+/* dissect the payload of a MUX-PDU
+ *
+ * tvb		buffer containing entire mux-pdu payload
+ * pinfo	info on the packet containing the last fragment of the MUX-PDU
+ * pkt_offset   offset within the block from the superdissector where the
+ *                MUX-PDU starts (must increase monotonically for constant
+ *                pinfo->fd->num)
+ * pdu_tree	dissection tree for the PDU
+ * call_info	data structure for h223 call
+ * mc		multiplex code for this PDU
+ * endOfMuxSdu	true if the end-of-sdu flag was set
+ */ 
+static void dissect_mux_payload( tvbuff_t *tvb, packet_info *pinfo, guint32 pkt_offset,
+                                 proto_tree *pdu_tree,
                                  h223_call_info* call_info, guint8 mc, gboolean endOfMuxSdu )
 {
     guint32 len = tvb_reported_length(tvb);
 
-    h223_mux_element* me = find_h223_mux_element( &(call_info->direction_data[pinfo->p2p_dir ? 0 : 1]), mc, pinfo->fd->num );
+    h223_mux_element* me = find_h223_mux_element( &(call_info->direction_data[pinfo->p2p_dir ? 0 : 1]), mc, pinfo->fd->num, pkt_offset );
 
     if( me ) {
         dissect_mux_payload_by_me_list( tvb, pinfo, pkt_offset, pdu_tree, call_info, me, 0, endOfMuxSdu );
@@ -802,7 +844,9 @@
  *
  * tvb		buffer containing mux-pdu, including header and closing flag
  * pinfo	packet info for packet containing the end of the mux-pdu
- * pkt_offset   offset within that packet of the start of the last fragment
+ * pkt_offset   offset within the block from the superdissector where the
+ *                MUX-PDU starts (must increase monotonically for constant
+ *                pinfo->fd->num)
  * h223_tree	dissection tree for h223 protocol; a single item will be added
  * 		(with a sub-tree)
  * call_info	h223 info structure for this h223 call
@@ -827,6 +871,11 @@
     proto_item *pdu_item = NULL;
     proto_tree *pdu_tree = NULL;
 
+#ifdef DEBUG_H223_FRAGMENTATION
+    g_debug("%u: dissecting complete H.223 MUX-PDU, pkt_offset %u, len %u",
+            pinfo->fd->num, pkt_offset, tvb_reported_length(tvb));
+#endif
+    
     switch(call_info->h223_level) {
         case 0: case 1:
             raw_hdr = tvb_get_guint8(tvb,0);
@@ -842,15 +891,25 @@
         case 2:
             raw_hdr = tvb_get_letoh24(tvb,0);
             errors = golay_errors(raw_hdr);
-            correct_hdr = ((errors == -1) ? raw_hdr : raw_hdr ^ (guint32)errors);
+            offset += 3;
+            len = tvb_length_remaining(tvb,offset)-2;
+
+            if(errors != -1) {
+                correct_hdr = raw_hdr ^ (guint32)errors;
     
-            mc = (guint8)(correct_hdr & 0xf);
-            mpl = (guint8)((correct_hdr >> 4) & 0xff);
+                mc = (guint8)(correct_hdr & 0xf);
+                mpl = (guint8)((correct_hdr >> 4) & 0xff);
 
-            offset += 3;
-            len = tvb_length_remaining(tvb,offset)-2;
-            closing_flag = tvb_get_ntohs(tvb,offset+len);
-            end_of_mux_sdu = (closing_flag==(0xE14D ^ 0xFFFF));
+                /* we should never have been called if there's not enough data in
+                 * available. */
+                DISSECTOR_ASSERT(len >= mpl);
+
+                closing_flag = tvb_get_ntohs(tvb,offset+len);
+                end_of_mux_sdu = (closing_flag==(0xE14D ^ 0xFFFF));
+            } else {
+                mc = 0;
+                mpl = len;
+            }
             break;
 
         case 3:
@@ -885,21 +944,22 @@
                     proto_tree_add_uint_format(hdr_tree, hf_h223_mux_rawhdr, tvb,
                                                0, 3, raw_hdr,
                                                "Raw value: 0x%06x (uncorrectable errors)", raw_hdr );
-                } else if( errors == 0 ) {
-                    proto_tree_add_uint_format(hdr_tree, hf_h223_mux_rawhdr, tvb,
-                                               0, 3, raw_hdr,
-                                               "Raw value: 0x%06x (correct)", raw_hdr );
                 } else {
-                    proto_tree_add_uint_format(hdr_tree, hf_h223_mux_rawhdr, tvb,
-                                               0, 3, raw_hdr,
-                                               "Raw value: 0x%06x (errors are 0x%06x)", raw_hdr, errors );
+                    if( errors == 0 ) {
+                        proto_tree_add_uint_format(hdr_tree, hf_h223_mux_rawhdr, tvb,
+                                                   0, 3, raw_hdr,
+                                                   "Raw value: 0x%06x (correct)", raw_hdr );
+                    } else {
+                        proto_tree_add_uint_format(hdr_tree, hf_h223_mux_rawhdr, tvb,
+                                                   0, 3, raw_hdr,
+                                                   "Raw value: 0x%06x (errors are 0x%06x)", raw_hdr, errors );
+                    }
+                    item = proto_tree_add_uint(hdr_tree,hf_h223_mux_correctedhdr,tvb,0,3,
+                                               correct_hdr);
+                    PROTO_ITEM_SET_GENERATED(item);
+                    proto_tree_add_uint(hdr_tree,hf_h223_mux_mc,tvb,0,1,mc);
+                    proto_tree_add_uint(hdr_tree,hf_h223_mux_mpl,tvb,0,2,mpl);
                 }
-                item = proto_tree_add_uint(hdr_tree,hf_h223_mux_correctedhdr,tvb,0,3,
-                                    correct_hdr);
-                PROTO_ITEM_SET_GENERATED(item);
-                
-                proto_tree_add_uint(hdr_tree,hf_h223_mux_mc,tvb,0,1,mc);
-                proto_tree_add_uint(hdr_tree,hf_h223_mux_mpl,tvb,0,2,mpl);
                 break;
 
             case 3:
@@ -909,9 +969,15 @@
         }
     }
 
-    pdu_tvb = tvb_new_subset(tvb, offset, len, mpl);
-    dissect_mux_payload(pdu_tvb,pinfo,offset+pkt_offset,pdu_tree,call_info,mc,end_of_mux_sdu);
-    offset += mpl;
+    if(mpl > 0) {
+        pdu_tvb = tvb_new_subset(tvb, offset, len, mpl);
+        if(errors != -1) {
+            dissect_mux_payload(pdu_tvb,pinfo,pkt_offset+offset,pdu_tree,call_info,mc,end_of_mux_sdu);
+        } else {
+            call_dissector(data_handle,pdu_tvb,pinfo,pdu_tree);
+        }
+        offset += mpl;
+    }
 
     /* any extra data in the PDU, beyond that indictated by the mpl, is
        dissected as data. */
@@ -941,42 +1007,45 @@
  */
 
 /* attempt to parse the header of a mux pdu */
-static void attempt_mux_level0_header_parse(h223_call_direction_data *dirdata)
+static gboolean attempt_mux_level0_header_parse(guint32 nbytes, guint32 hdr, guint32 *minlen)
 {
     /* level 0 isn't byte-aligned, so is a complete pain to implement */
     DISSECTOR_ASSERT_NOT_REACHED();
-    dirdata = dirdata;
+    nbytes = nbytes;
+    hdr=hdr;
+    minlen=minlen;
+    return FALSE;
 }
 
-static void attempt_mux_level1_header_parse(h223_call_direction_data *dirdata)
+static gboolean attempt_mux_level1_header_parse(guint32 nbytes, guint32 hdr, guint32 *minlen)
 {
-    guint32 hdr;
+    /* this is untested */
+    DISSECTOR_ASSERT_NOT_REACHED();
     
-    if(dirdata->current_pdu_read != 2)
-        return;
+    if(nbytes < 2)
+        return FALSE;
 
-    hdr = dirdata->header_buf & 0xffff;
+    hdr &= 0xffff;
     /* don't interpret a repeated hdlc as a header */
     if(hdr == 0xE14D)
-        return;
+        return FALSE;
 
     /* + 1 byte of header and 2 bytes of closing HDLC */
-    dirdata -> current_pdu_minlen = (guint8)((hdr >> 12) & 0xff) + 3;
-    dirdata -> current_pdu_header_parsed = TRUE;
+    *minlen = (guint8)((hdr >> 12) & 0xff) + 3;
+    return TRUE;
 }
 
-static void attempt_mux_level2_3_header_parse(h223_call_direction_data *dirdata)
+static gboolean attempt_mux_level2_3_header_parse(guint32 nbytes, guint32 hdr, guint32 *minlen)
 {
-    guint32 hdr;
     gint32 errors;
     
-    if(dirdata->current_pdu_read != 3)
-        return;
+    if(nbytes < 3)
+        return FALSE;
 
     /* + 3 bytes of header and 2 bytes of closing HDLC */
-    dirdata -> current_pdu_minlen = 5;
+    *minlen = 5;
 
-    hdr = dirdata->header_buf;
+    /* bah, we get the header in the wrong order */
     hdr =
         ((hdr & 0xFF0000) >> 16) |
         (hdr & 0x00FF00) |
@@ -985,20 +1054,20 @@
     errors = golay_errors(hdr);
     if(errors != -1) {
         hdr ^= errors;
-        dirdata -> current_pdu_minlen += ((hdr >> 4) & 0xff);
+        *minlen += ((hdr >> 4) & 0xff);
     }
-    
-    dirdata -> current_pdu_header_parsed = TRUE;
+
+    return TRUE;
 }
 
-static void (* const attempt_mux_header_parse[])(h223_call_direction_data *dirdata) = {
+static gboolean (* const attempt_mux_header_parse[])(guint32 nbytes, guint32 header_buf, guint32 *minlen) = {
     attempt_mux_level0_header_parse,
     attempt_mux_level1_header_parse,
     attempt_mux_level2_3_header_parse,
     attempt_mux_level2_3_header_parse
 };
 
-static gboolean h223_mux_check_hdlc(int h223_level, h223_call_direction_data *dirdata)
+static gboolean h223_mux_check_hdlc(int h223_level, guint32 nbytes, guint32 tail_buf)
 {
     guint32 masked;
 
@@ -1010,12 +1079,12 @@
             break;
 
         case 1:
-            masked = dirdata->tail_buf & 0xffff;
-            return masked == 0xE14D;
+            masked = tail_buf & 0xffff;
+            return nbytes >= 2 && masked == 0xE14D;
 
         case 2: case 3:
-            masked = dirdata->tail_buf & 0xffff;
-            return masked == 0xE14D || masked == (0xE14D ^ 0xFFFF);
+            masked = tail_buf & 0xffff;
+            return nbytes >= 2 && (masked == 0xE14D || masked == (0xE14D ^ 0xFFFF));
 
         default:
             DISSECTOR_ASSERT_NOT_REACHED();
@@ -1023,108 +1092,85 @@
     }
 }
 
-/* read a pdu (or the end of a pdu) from the tvb, and dissect it
+/* read a pdu (or the start of a pdu) from the tvb, and dissect it
  *
- * returns an offset to the next byte
- *
- * *pdu_found is set TRUE if a pdu was found, or FALSE if we reached the
- * end of the tvb without completing one.
+ * returns the number of bytes processed, or the negative of the number of
+ * extra bytes needed, or zero if we don't know yet
  */
 
-static guint32 dissect_mux_pdu_fragment( tvbuff_t *tvb, guint32 start_offset, packet_info * pinfo,
-					 guint32* pkt_offset,
-					 proto_tree *tree,
-					 proto_tree **h223_tree_p,
-					 h223_call_info *call_info,
-					 gboolean *pdu_found)
+static gint dissect_mux_pdu_fragment( tvbuff_t *tvb, guint32 start_offset, packet_info * pinfo,
+                                      proto_tree *h223_tree,
+                                      h223_call_info *call_info)
 {
-    proto_item *h223_item = NULL;
-    proto_tree *volatile h223_tree = *h223_tree_p;
     tvbuff_t *volatile next_tvb;
     volatile guint32 offset = start_offset;
     gboolean more_frags = TRUE;
-	proto_tree *pdu_tree;
 
-    h223_call_direction_data *dirdata = &call_info -> direction_data[pinfo->p2p_dir ? 0 : 1];
+    gboolean header_parsed = FALSE;
+    guint32 header_buf = 0, tail_buf = 0;
+    guint32 pdu_minlen = 0;
+    
 
-    dirdata -> current_pdu_read = 0;
-    dirdata -> current_pdu_minlen = 0;
-    dirdata -> current_pdu_header_parsed = FALSE;
-
+#ifdef DEBUG_H223_FRAGMENTATION
+    g_debug("%d: dissecting H.223 PDU, start_offset %u, %u bytes left",
+            pinfo->fd->num,start_offset, tvb_reported_length_remaining( tvb, start_offset ));
+#endif
+    
     while( more_frags && offset < tvb_reported_length( tvb )) {
         guint8 byte = tvb_get_guint8(tvb, offset++);
-        dirdata -> current_pdu_read++;
         
         /* read a byte into the header buf, if necessary */
-        if(dirdata -> current_pdu_read <= 4) {
-            dirdata -> header_buf <<= 8;
-            dirdata -> header_buf |= byte;
+        if((offset-start_offset) <= 4) {
+            header_buf <<= 8;
+            header_buf |= byte;
         }
 
         /* read the byte into the tail buf */
-        dirdata -> tail_buf <<= 8;
-        dirdata -> tail_buf |= byte;
+        tail_buf <<= 8;
+        tail_buf |= byte;
 
         /* if we haven't parsed the header yet, attempt to do so now */
-        if(!dirdata -> current_pdu_header_parsed)
+        if(!header_parsed)
             /* this sets current_pdu_header parsed if current_pdu_read == 3 */
-            (attempt_mux_header_parse[call_info->h223_level])(dirdata);
+            header_parsed = (attempt_mux_header_parse[call_info->h223_level])
+                (offset-start_offset,header_buf,&pdu_minlen);
 
-        if(dirdata -> current_pdu_read >= dirdata -> current_pdu_minlen) {
-            if(h223_mux_check_hdlc(call_info->h223_level,dirdata)) {
-                dirdata -> current_pdu_minlen = 0;
-                dirdata -> current_pdu_read = 0;
-                dirdata -> current_pdu_header_parsed = FALSE;
+        /* if we have successfully parsed the header, we have sufficient data,
+         * and we have found the closing hdlc, we are done here */
+        if(header_parsed && (offset-start_offset) >= pdu_minlen) {
+            if(h223_mux_check_hdlc(call_info->h223_level,offset-start_offset,tail_buf)) {
                 more_frags = FALSE;
             }
         }
     }
 
     if( more_frags ) {
-        /* offset = tvb_reported length now */
-        pinfo->desegment_offset = offset - dirdata->current_pdu_read;
-        if(dirdata->current_pdu_read > dirdata->current_pdu_minlen)
-            pinfo->desegment_len = 1;
-        else
-            pinfo->desegment_len = dirdata->current_pdu_minlen - dirdata->current_pdu_read;
-        return offset;
-    }
-
-    if(!*h223_tree_p) {
-        /* add the 'h223' tree to the main tree */
-        if (tree) {
-            h223_item = proto_tree_add_item (tree, proto_h223, tvb, 0, -1, FALSE);
-            h223_tree = proto_item_add_subtree (h223_item, ett_h223);
-            *h223_tree_p = h223_tree;
+        if(pdu_minlen <= (offset-start_offset)) {
+            /* we haven't found the closing hdlc yet, but we don't know how
+             * much more we need */
+#ifdef DEBUG_H223_FRAGMENTATION
+            g_debug("\tBailing, requesting more bytes");
+#endif
+            return 0;
+        } else {
+            uint needed = pdu_minlen-(offset-start_offset);
+#ifdef DEBUG_H223_FRAGMENTATION
+            g_debug("\tBailing, requesting %i-%i=%u more bytes", pdu_minlen,(offset-start_offset),needed);
+#endif
+            return -needed;
         }
     }
-    *pdu_found = TRUE;
 
     /* create a tvb for the fragment */
     next_tvb = tvb_new_subset(tvb, start_offset, offset-start_offset,
 			      offset-start_offset);
 
 
-    *pkt_offset += tvb_reported_length( next_tvb );
-
-    /* the first PDU isn't real H.223 data. */
-    if( dirdata->first_pdu ) {
-        dirdata->first_pdu = FALSE;
-        pdu_tree = NULL;
-        if( h223_tree ) {
-            proto_item *pdu_item = proto_tree_add_item (h223_tree, hf_h223_non_h223_data, tvb, 0, -1, FALSE);
-            pdu_tree = proto_item_add_subtree (pdu_item, ett_h223_non_h223_data);
-        }
-        call_dissector(data_handle,tvb, pinfo, pdu_tree);
-        return offset;
-    }
-
     /* we catch boundserrors on the pdu so that errors on an
      * individual pdu don't screw up the whole of the rest of the
      * stream */
-    pdu_offset = *pkt_offset - tvb_reported_length( next_tvb );
     TRY {
-        dissect_mux_pdu( next_tvb, pinfo, *pkt_offset - tvb_reported_length( next_tvb ), h223_tree, call_info);
+        dissect_mux_pdu( next_tvb, pinfo, start_offset, h223_tree, call_info);
     }
 
     CATCH2(BoundsError,ReportedBoundsError) {
@@ -1137,31 +1183,29 @@
 
     ENDTRY;
 
-    return offset;
+    return (offset-start_offset);
 }
 
 /************************************************************************************
  *
  * main dissector entry points
  */
-    
+
+/* dissects PDUs from the tvb
+ *
+ * Updates desegment_offset and desegment_len if the end of the data didn't
+ * line up with the end of a pdu.
+ */
 static void dissect_h223 (tvbuff_t * tvb, packet_info * pinfo, proto_tree * tree)
 {
     proto_tree *h223_tree = NULL;
+    proto_item *h223_item = NULL;
     h223_call_info *call_info = NULL;
     guint32 offset = 0;
 
-    /* pkt_offset becomes different from offset if we reassemble a pdu:
-     *
-     * before: offset = a, pkt_offset = b
-     * offset = dissect_h223_mux_pdu_fragment(ending fragment, offset, &pkt_offset)
-     * after: offset = a + sizeof(ending frament), pkt_offset = b + sizeof(reassembled pdu)
-     *
-     * This lets us get a value "pkt_offset + offset_into_pdu" which will never decrease
-     * as we walk through a packet
-     */
-    guint32 pkt_offset = 0;
-    gboolean pdu_found = FALSE;
+    /* set up the protocol and info fields in the summary pane */
+    if (check_col (pinfo->cinfo, COL_PROTOCOL))
+	col_set_str (pinfo->cinfo, COL_PROTOCOL, PROTO_TAG_H223);
 
     if (check_col (pinfo->cinfo, COL_INFO))
 	col_clear (pinfo->cinfo, COL_INFO);
@@ -1169,21 +1213,46 @@
     /* find or create the call_info for this call */
     call_info = find_or_create_call_info(pinfo);
 
+    /* add the 'h223' tree to the main tree */
+    if (tree) {
+        h223_item = proto_tree_add_item (tree, proto_h223, tvb, 0, -1, FALSE);
+        h223_tree = proto_item_add_subtree (h223_item, ett_h223);
+    }
+
     while( offset < tvb_reported_length( tvb )) {
-	gboolean pdu_found_this_fragment = FALSE;
-	offset = dissect_mux_pdu_fragment( tvb, offset, pinfo, &pkt_offset, tree,
-					   &h223_tree, call_info,
-					   &pdu_found_this_fragment );
-	if( pdu_found_this_fragment )
-	    pdu_found = TRUE;
+	int res = dissect_mux_pdu_fragment( tvb, offset, pinfo,
+	                                    h223_tree, call_info);
+	if(res <= 0) {
+	    /* the end of the tvb held the start of a PDU */
+	    pinfo->desegment_offset = offset;
+
+	    /* if res != 0, we actually know how much more data we need for a
+	     * PDU.
+	     *
+	     * However, if we return that, it means that we get called twice
+	     * for the next packet; this makes it hard to tell how far throught
+	     * the stream we are and we have to start messing about with
+	     * getting the seqno from the superdissector's private data. So we
+	     * don't do that.
+	     *
+	     * pinfo->desegment_len = (res == 0 ? DESEGMENT_ONE_MORE_SEGMENT : -res);
+	     */
+	    pinfo -> desegment_len = DESEGMENT_ONE_MORE_SEGMENT;
+
+	    if(h223_item) {
+		/* shrink the h223 protocol item such that it only includes the
+		 * bits we dissected */
+		proto_item_set_len(h223_item,offset);
+	    }
+
+	    if(offset == 0) {
+		if(check_col (pinfo->cinfo, COL_INFO))
+		    col_set_str (pinfo->cinfo, COL_INFO, "(No complete PDUs)");
+	    }
+	    return;
+	}
+	offset += res;
     }
-
-    if( !pdu_found && check_col (pinfo->cinfo, COL_INFO))
-	col_set_str (pinfo->cinfo, COL_INFO, "(No complete PDUs)");
-	
-    /* set up the protocol and info fields in the summary pane */
-    if (check_col (pinfo->cinfo, COL_PROTOCOL))
-	col_set_str (pinfo->cinfo, COL_PROTOCOL, PROTO_TAG_H223);
 }
 
 /* H.223 specifies that the least-significant bit is transmitted first;
@@ -1366,13 +1435,17 @@
 	    "", HFILL }},
 
 	{ &hf_h223_al2,
-	  { "H.223 AL2", "h223.al2", FT_NONE, BASE_NONE, NULL, 0x0,
+	  { "H.223 AL2", "h223.al2", FT_BOOLEAN, BASE_NONE, NULL, 0x0,
 	    "H.223 AL-PDU using AL2", HFILL }},
 
 	{ &hf_h223_al2_sequenced,
-	  { "H.223 AL2 sequenced", "h223.al2.sequenced", FT_BOOLEAN, BASE_NONE, NULL, 0x0,
-	    "", HFILL }},
+	  { "H.223 sequenced AL2", "h223.sequenced_al2", FT_NONE, BASE_NONE, NULL, 0x0,
+	    "H.223 AL-PDU using AL2 with sequence numbers", HFILL }},
 
+	{ &hf_h223_al2_unsequenced,
+	  { "H.223 unsequenced AL2", "h223.unsequenced_al2", FT_NONE, BASE_NONE, NULL, 0x0,
+	    "H.223 AL-PDU using AL2 without sequence numbers", HFILL }},
+
 	{ &hf_h223_al2_seqno,
 	  { "Sequence Number", "h223.al2.seqno", FT_UINT8, BASE_DEC, NULL, 0x0,
 	    "H.223 AL2 sequence number", HFILL }},