d_tcp_decode_opt_sack() wrong

Bug #1502170 reported by Klaus Warnke on 2015-10-02
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Status tracked in Rohc-main
Didier Barvaux
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
for testing. Found at

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.

Klaus Warnke

Didier Barvaux (didier-barvaux) wrote :


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


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 :


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.


4.7. Expressions
   o There are no bitwise operators.


This make no sense for this:

    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

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

Klaus Warnke

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

Other bug subscribers