d_tcp_decode_opt_sack() wrong

Bug #1502170 reported by Klaus Warnke on 2015-10-02
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
rohc
Status tracked in Rohc-main
Rohc-1.7.x
Medium
Didier Barvaux
Rohc-main
Medium
Didier Barvaux

Bug Description

Hello Didier,

I guess, there is a conformance problem in the SACK option decompressing.
The function d_tcp_decode_opt_sack() uses the ack_num for
all blocks as reference/base, but it is for the first block only.
The next block uses the value for its previous block a base.

See for two block for example:

  COMPRESSED sack2_list_item {
    discriminator =:= '00000010';
    block_1 =:= sack_block(ack_value);
    block_2 =:= sack_block(block_1.UVALUE && 0xFFFFFFFF);
    ENFORCE(length.UVALUE == 18);
  }

For the first block it is the ack_value, but for the second one
it is block_1.start.
Could you agree?
By the way, I implemented it wrong too.
I used always the previous value, that is, the block_1.end as
reference. I assumed the values are chained.

I propose the following patch:

diff --git a/src/decomp/d_tcp.c b/src/decomp/d_tcp.c
index 9ededdc..37a8950 100644
--- a/src/decomp/d_tcp.c
+++ b/src/decomp/d_tcp.c
@@ -2654,7 +2655,8 @@ static void d_tcp_decode_opt_sack(const struct rohc_decomp_ctxt *const context,

  for(i = 0; i < bits.blocks_nr; i++)
  {
- const uint32_t block_start = ack_num + bits.blocks[i].block_start;
+ const uint32_t base = ((i == 0) ? ack_num : rohc_ntoh32(decoded->blocks[i-1].block_start));
+ const uint32_t block_start = base + bits.blocks[i].block_start;
   const uint32_t block_end = block_start + bits.blocks[i].block_end;
   decoded->blocks[i].block_start = rohc_hton32(block_start);
   decoded->blocks[i].block_end = rohc_hton32(block_end);

I used
http://www.snaketrap.co.uk/pcap/hptcp.pcap
for testing. Found at
http://www.netresec.com/?page=PcapFiles

It is a huge pcap file, with 364372 packets.
Most TCP, mixed with DNS (UDP) and ARP.
One packet has more the 16 options, is therefore not compressible.

Best,
Klaus Warnke

Didier Barvaux (didier-barvaux) wrote :

Klaus,

You're right. Good catch!

You talk about block.end but your patch proposal uses rohc_ntoh32(decoded->blocks[i-1].block_start). My understanding of the RFC (see below) is that block end shall be used. Do you agree?

I interpret the following snippet of RFC 6846 as block end:
  block_1.UVALUE && 0xFFFFFFFF

Regards,
Didier

Didier Barvaux (didier-barvaux) wrote :

On a side note, I reported an errata to RFC 6846: block_3 in sack3_irregular() uses block_1 as reference while it should use block_2.

Klaus Warnke (k-warnke) wrote :

Hm,

the operator "&&" is defined in rfc 4997 as:

4.7.4. Boolean Operators
   o &&, for logical "and". Returns true if both arguments are true.
      Returns false otherwise.

and

4.7. Expressions
   o There are no bitwise operators.

;-)

This make no sense for this:

tcp_opt_sack(ack_value)
{
  UNCOMPRESSED {
    block_1 [ 64 ];
  }

  COMPRESSED sack2_list_item {
    block_2 =:= sack_block(block_1.UVALUE && 0xFFFFFFFF);
  }
}

There are no boolean values involved at all.
However, I interpreted "&&" as the C operator "&" (bitwise and).
Then I think the "&& 0xFFFFFFFF" should use the lower 32bit
from the 64bit sized block_1 and omit the higher 32bit.
That is the way how it works in C.
And if

  UNCOMPRESSED {
    block_start [ 32 ];
    block_end [ 32 ];
  }

are ordered from left to right, then
the lower 32bit are block_end.
Your are right.
Then I thought too long about this on a Friday evening...
This is the correct version:

  const uint32_t base = ((i == 0) ? ack_num : rohc_ntoh32(decoded->blocks[i-1].block_end));

Best,
Klaus Warnke

To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers