d_tcp_decode_opt_sack() wrong
Affects | Status | Importance | Assigned to | Milestone | ||
---|---|---|---|---|---|---|
rohc | Status tracked in Rohc-main | |||||
Rohc-1.7.x |
Won't Fix
|
Medium
|
Didier Barvaux | |||
Rohc-main |
Fix Released
|
Medium
|
Didier Barvaux |
Bug Description
Hello Didier,
I guess, there is a conformance problem in the SACK option decompressing.
The function d_tcp_decode_
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(
block_2 =:= sack_block(
ENFORCE(
}
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/
index 9ededdc..37a8950 100644
--- a/src/decomp/
+++ b/src/decomp/
@@ -2654,7 +2655,8 @@ static void d_tcp_decode_
for(i = 0; i < bits.blocks_nr; i++)
{
- const uint32_t block_start = ack_num + bits.blocks[
+ const uint32_t base = ((i == 0) ? ack_num : rohc_ntoh32(
+ const uint32_t block_start = base + bits.blocks[
const uint32_t block_end = block_start + bits.blocks[
decoded-
decoded-
I used
http://
for testing. Found at
http://
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
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