TCP: sack_unchanged_irregular handling wrong

[Klaus Warnke]

Hello,

in case of no sack data are transmitted in the irregular chain,
all SACK data are omitted on rebuild.
The test case is ipv4_tcp_afl30-mss-or-ws-opts-in-irregular-chain.
The first and the second packet are equal, therefore no
SACK data needed to transmitted again.
The RoHC pakcet are:

[DEBUG] [rohc_traces_internal.c:100 rohc_dump_buf()] compressed data, max 100 bytes (50 bytes):
[DEBUG] [rohc_traces_internal.c:109 rohc_dump_buf()] fd 06 34 00 06 c0 a8 02 02 5b 79 32 85 be 63 00
[DEBUG] [rohc_traces_internal.c:109 rohc_dump_buf()] 50 06 00 40 45 35 30 02 00 01 b7 00 cb 35 39 08
[DEBUG] [rohc_traces_internal.c:109 rohc_dump_buf()] 50 d7 05 ad e8 b0 05 b4 01 ff ff d6 8e 00 a9 72
[DEBUG] [rohc_traces_internal.c:129 rohc_dump_buf()] 00 07

[DEBUG] [rohc_traces_internal.c:100 rohc_dump_buf()] compressed data, max 100 bytes (10 bytes):
[DEBUG] [rohc_traces_internal.c:129 rohc_dump_buf()] fa 22 00 04 9f 45 35 50 d7 00

The discriminator while calling tcp_sack.c d_tcp_sack_parse() is 0.
No data are transmitted, but opt_sack->blocks_nr is overwritten with 0.
While rebuilding in d_tcp_build_sack() no data are rebuild.
I suggest the patch:

diff --git a/src/decomp/schemes/tcp_sack.c b/src/decomp/schemes/tcp_sack.c
index ade42df..e1b8ea3 100644
--- a/src/decomp/schemes/tcp_sack.c
+++ b/src/decomp/schemes/tcp_sack.c
@@ -107,8 +107,11 @@ int d_tcp_sack_parse(const struct rohc_decomp_ctxt *const context,
                     "end bits = 0x%08x", i + 1, opt_sack->blocks[i].block_start,
                     opt_sack->blocks[i].block_end);
  }
- opt_sack->blocks_nr = discriminator;
-
+ if (discriminator > 0)
+ {
+ opt_sack->blocks_nr = discriminator;
+ }
+
  return (data_len - remain_data_len);

 error:

to avoid the error in case of sack_unchanged_irregular was used.

Best,
Klaus Warnke

[Didier Barvaux (didier-barvaux)]

Klaus,

You're right, once more. The case with unchanged SACK option is not handled correctly. Thank you for the report!

However, I'm afraid that your fix proposal is not reliable. If discriminator is 0, you doesn't record anything into the struct d_tcp_opt_ctxt, so you keep the old values. By chance the old values seem to be the values of the previous SACK option, so it gives the good result. I would prefer to record an indication that the SACK option shall be retrieved from context, and then during the decoding phase retrieve the values from context. Some other options are implemented this way when static values are not transmitted. I'll implement something this way asap.

Regards,
Didier

[Klaus Warnke (k-warnke)]

I agree, this patch does not work, if a SACK option disappears for one packet and appears again.
This occurs in the 4th packet.
This next try works for me.
It uses blocks_nr as an "is_static" indicator.
To be more explicit, you can introduce it
and set it in the _dyn and _irreg functions accordingly.
However, this patch uses the context values while building,
if no bits are received:

diff --git a/src/decomp/d_tcp_opts_list.c b/src/decomp/d_tcp_opts_list.c
index 0fef714..5b84b74 100644
--- a/src/decomp/d_tcp_opts_list.c
+++ b/src/decomp/d_tcp_opts_list.c
@@ -1176,10 +1184,26 @@ static bool d_tcp_build_sack(const struct rohc_decomp_ctxt *const context,
                              struct rohc_buf *const uncomp_packet,
                              size_t *const opt_len)
 {
- const sack_block_t *const blocks = decoded->opt_sack_blocks.blocks;
- const size_t blocks_nr = decoded->opt_sack_blocks.blocks_nr;
- const size_t blocks_len = sizeof(sack_block_t) * blocks_nr;
- const size_t sack_len = 2 + blocks_len;
+ sack_block_t const *blocks;
+ size_t blocks_nr;
+ size_t blocks_len;
+ size_t sack_len;
+
+ if (decoded->opt_sack_blocks.blocks_nr > 0)
+ {
+ blocks = decoded->opt_sack_blocks.blocks;
+ blocks_nr = decoded->opt_sack_blocks.blocks_nr;
+ }
+ else
+ {
+ const struct d_tcp_context *const tcp_context = context->persist_ctxt;
+
+ blocks = tcp_context->opt_sack_blocks.blocks;
+ blocks_nr = tcp_context->opt_sack_blocks.blocks_nr;
+ }
+
+ blocks_len = sizeof(sack_block_t) * blocks_nr;
+ sack_len = 2 + blocks_len;

  if(rohc_buf_avail_len(*uncomp_packet) < sack_len)
  {

And this avoid overwriting the context with empty values,
if no received.

diff --git a/src/decomp/d_tcp.c b/src/decomp/d_tcp.c
index 9ededdc..94ac4b9 100644
--- a/src/decomp/d_tcp.c
+++ b/src/decomp/d_tcp.c
@@ -3389,8 +3390,11 @@ static void d_tcp_update_ctxt(struct rohc_decomp_ctxt *const context,
   }
   else if(opt_index == TCP_INDEX_SACK)
   {
- memcpy(&tcp_context->opt_sack_blocks, &decoded->opt_sack_blocks,
- sizeof(struct d_tcp_opt_sack));
+ if (decoded->opt_sack_blocks.blocks_nr > 0)
+ {
+ memcpy(&tcp_context->opt_sack_blocks, &decoded->opt_sack_blocks,
+ sizeof(struct d_tcp_opt_sack));
+ }
   }
  }
 }

And don't use the d_tcp_sack_parse() hack.

Feel free to replace blocks_nr with a is_static variable.
These are the 7 RoHC packet for testing, if you want:

[DEBUG] [rohc_decomp.c:779 rohc_decompress3()] decompress the 50-byte packet #1
[DEBUG] [rohc_traces_internal.c:100 rohc_dump_buf()] compressed data, max 100 bytes (50 bytes):
[DEBUG] [rohc_traces_internal.c:109 rohc_dump_buf()] fd 06 34 00 06 c0 a8 02 02 5b 79 32 85 be 63 00
[DEBUG] [rohc_traces_internal.c:109 rohc_dump_buf()] 50 06 00 40 45 35 30 02 00 01 b7 00 cb 35 39 08
[DEBUG] [rohc_traces_internal.c:109 rohc_dump_buf()] 50 d7 05 ad e8 b0 05 b4 01 ff ff d6 8e 00 a9 72
[DEBUG] [rohc_traces_internal.c:129 rohc_dump_buf()] 00 07

[DEBUG] [rohc_decomp.c:779 rohc_decompress3()] decompress the 10-byte packet #2
[DEBUG] [rohc_traces_internal.c:100 rohc_dump_buf()] compressed data, max 100 bytes (10 bytes):
[DEBUG] [rohc_traces_internal.c:129 rohc_dump_buf()] fa 22 00 04 9f...

[Klaus Warnke (k-warnke)]

#4 and #7 are larger than 100bytes...

#4:
0000 b3 be 48 02 68 0b 36 03
0008 00 c0 41 d6 8e 04 02 00
0010 04 00 0b 43 00 00 00 00
0018 00 00 00 00 ff 7f 00 00
0020 01 00 00 00 b3 20 88 50
0028 50 ab 04 00 4a 00 00 00
0030 4a 00 00 00 00 0b 6b 7e
0038 54 71 00 19 7e b2 12 af
0040 08 00 45 00 00 3c 45 35
0048 40 00 40 06 a4 de c0 a8
0050 02 02 5b 79 32 85 be 63
0058 00 50 b7 00 cb 35 00 00
0060 00 00 a0 02 39 08 50 d7
0068 00 00 02 04 05 b4 88 50
0070 2b 0a ff d6 8e 00 00 00
0078 00 00 01 03 03 07

#7
0000 fa 27 74 08 b3 35 00 00
0008 00 00 39 08 30 05 25 60
0010 30 50 d7 00 b3 20 88 50
0018 e7 31 05 00 4a 00 00 00
0020 4a 00 00 00 00 0b 6b 7e
0028 54 71 00 19 7e b2 12 af
0030 08 00 45 00 00 3c 45 35
0038 40 00 40 06 a4 de c0 a8
0040 02 02 5b 79 32 85 be 63
0048 00 50 b7 00 cb 35 00 00
0050 00 00 a0 02 39 08 50 d7
0058 00 00 02 04 05 b4 04 02
0060 6f 6e 6e 65 63 74 69 6f
0068 6e 3a 20 4b 65 65 70 2d
0070 41 6c 69 76 65 0d

[Didier Barvaux (didier-barvaux)]

Klaus,

Your patch looks great, thank you!

Regards,
Didier

[Didier Barvaux (didier-barvaux)]

And, of course, thank you for your interoperability tests :)

[Klaus Warnke (k-warnke)]

Your welcome!
Using your decompressor helped me to find some bugs in my compressor.
The packet type decision algorithm is not easy...
And thanks for providing these test data.

Regards,
Klaus Warnke

Btw:
In C it is possible to assign struct variables direct, there is no need to use memcpy().
You can use, for example as

memcpy(&tcp_context->opt_sack_blocks,
    &decoded->opt_sack_blocks,
    sizeof(struct d_tcp_opt_sack));

this direct:

    tcp_context->opt_sack_blocks = decoded->opt_sack_blocks;

if the struct types are the same and its definition is known.
If not, the compiler will complain.
This does not work for array types, even there are known.
Or you put the array into a struct, then it works...

[Didier Barvaux (didier-barvaux)]

Klaus,

Great that it improved your implementation too :) Tell me if I could help you for some other interoperability tests (my compressor and your decompressor for example).

I'm avoiding the direct struct copies because it doesn't work well on some archs with non-aligned memory. Using memcpy works everywhere. I didn't check whether there is a performance penalty or not...

Regards,
Didier

[Didier Barvaux (didier-barvaux)]

Fix pushed on master.