BinPac generated code: checking out-of-bound too early

Development
1

Hi,

I’m writing a BinPac flowunit analyzer, a PDU is like below:

type test_pdu = record {
lenAB : uint32; # length of rest of data
lenA : uint16; # length of dataA
dataA : bytestring &length = lenA;
dataB : bytestring &length = (lenAB - 2 - lenA);
} &byteorder=bigendian &length=(lenAB + 4);

There are 2 problems:

  1. binpac failed to compile (cannot handle incremental input) if I remove &length=(lenAB - 2 -lenA), although the overall length of the PDU can be calculated using the 4 field length

  2. the generated parser seems to check out-of-bound of lenA field too early:

1577 bool test_pdu::ParseBuffer(flow_buffer_t t_flow_buffer)
1578 {
1579 bool t_val_parsing_complete;
1580 t_val_parsing_complete = false;
1581 const_byteptr t_begin_of_data = t_flow_buffer->begin();
1582 const_byteptr t_end_of_data = t_flow_buffer->end();
1583 switch ( buffering_state_ )
1584 {
1585 case 0:
1586 if ( buffering_state_ == 0 )
1587 {
1588 t_flow_buffer->NewFrame(4, false);
1589 buffering_state_ = 1;
1590 }
1591 buffering_state_ = 1;
1592 break;
1593 case 1:
1594 {
1595 buffering_state_ = 2;
1596 // Checking out-of-bound for “test_pdu:lenA”
1597 if ( (t_begin_of_data + 4) + (2) > t_end_of_data || (t_begin_of_data + 4) + (2) < (t_begin_of_data + 4) )
1598 {
1599 // Handle out-of-bound condition
1600 throw binpac::ExceptionOutOfBound(“test_pdu:lenA”,
1601 (4) + (2),
1602 (t_end_of_data) - (t_begin_of_data));
1603 }
1604 // Parse “lenAB”
1605 lenAB_ = FixByteOrder(byteorder(), *((uint32 const *) (t_begin_of_data)));
1606 // Evaluate ‘let’ and ‘withinput’ fields
1607 t_flow_buffer->GrowFrame( ( lenAB() + 4 ) );
1608 }
1609 break;

Since we only make a new frame of length 4 in line #1588 (the flow buffer will not grow to full size until line #1607), the test in line #1597 will be evaluated to true and the parsing will fail.

What did I missed? Thanks in advance.

Best regards,
Song

2

I’d try reworking this, so that you have lenAB, and then another record for the rest of the data, and just setting the length of that record to lenAB. Does that work?

—Vlad

3

Thanks Vlad.

I tried as you suggestd and it works:

type test_pdu = record {
lenAB : uint32;
pduAB : test_pdu_ab(lenAB);
} &byteorder=bigendian &length=(lenAB + 4); # fail to compile without &length

type test_pdu_ab(len: uint32) = record {
lenA : uint16;
dataA : bytestring &length = lenA;
dataB : bytestring &length = (len - 2 - lenA);
} &byteorder=bigendian; # do not need &length here

The result is correct:

1577 bool test_pdu::ParseBuffer(flow_buffer_t t_flow_buffer)
1578 {
1579 bool t_val_parsing_complete;
1580 t_val_parsing_complete = false;
1581 const_byteptr t_begin_of_data = t_flow_buffer->begin();
1582 const_byteptr t_end_of_data = t_flow_buffer->end();
1583 switch ( buffering_state_ )
1584 {
1585 case 0:
1586 if ( buffering_state_ == 0 )
1587 {
1588 t_flow_buffer->NewFrame(4, false);
1589 buffering_state_ = 1;
1590 }
1591 buffering_state_ = 1;
1592 break;
1593 case 1:
1594 {
1595 buffering_state_ = 2;
1596 // Checking out-of-bound for “test_pdu:lenAB”
1597 if ( t_begin_of_data + (4) > t_end_of_data || t_begin_of_data + (4) < t_begin_of_data )
1598 {
1599 // Handle out-of-bound condition
1600 throw binpac::ExceptionOutOfBound(“test_pdu:lenAB”,
1601 (0) + (4),
1602 (t_end_of_data) - (t_begin_of_data));
1603 }
1604 // Parse “lenAB”
1605 lenAB_ = FixByteOrder(byteorder(), *((uint32 const *) (t_begin_of_data)));
1606 // Evaluate ‘let’ and ‘withinput’ fields
1607 t_flow_buffer->GrowFrame( ( lenAB() + 4 ) );
1608 }
1609 break;
1610 case 2:
1611 BINPAC_ASSERT(t_flow_buffer->ready());
1612 if ( t_flow_buffer->ready() )
1613 {
1614
1615 // Parse “pduAB”
1616 pduAB_ = new test_pdu_ab(lenAB());
1617 int t_pduAB__size;
1618 t_pduAB__size = pduAB_->Parse((t_begin_of_data + 4), t_end_of_data);
…
…
1649
1650 int test_pdu_ab::Parse(const_byteptr const t_begin_of_data, const_byteptr const t_end_of_data)
1651 {
1652 // Checking out-of-bound for “test_pdu_ab:lenA”
1653 if ( t_begin_of_data + (2) > t_end_of_data || t_begin_of_data + (2) < t_begin_of_data )
1654 {
1655 // Handle out-of-bound condition
1656 throw binpac::ExceptionOutOfBound(“test_pdu_ab:lenA”,
1657 (0) + (2),
1658 (t_end_of_data) - (t_begin_of_data));
1659 }
1660 // Parse “lenA”
1661 lenA_ = FixByteOrder(byteorder(), *((uint16 const *) (t_begin_of_data)));
1662 // Evaluate ‘let’ and ‘withinput’ fields
1663
1664 // Parse “dataA”
1665 int t_dataA__size;
1666 t_dataA__size = lenA();
1667 // Checking out-of-bound for “test_pdu_ab:dataA”
1668 if ( (t_begin_of_data + 2) + (t_dataA__size) > t_end_of_data || (t_begin_of_data + 2) + (t_dataA__size) < (t_begin_of_data + 2) )
1669 {

Does it necessary to do those out-of-bound checks like in line #1597 #1653 #1668? I think we have already got filled frame with correct size when these checks are performed.

Thanks again and best regards,
Song

------------------ Original ------------------