d_tcp_decode_opt_sack() wrong

[Klaus Warnke]

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)]

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)]

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)]

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

[Didier Barvaux (didier-barvaux)]

Fixed on branch dev_improve_get_captures_script: https://github.com/didier-barvaux/rohc/commit/447e78f05c898a99517cf99b043d7d24206c6905