IP_ID_BEHAVIOR_SEQ_SWAP decode issue

Bug #1506540 reported by Klaus Warnke on 2015-10-15
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

There is conformance issue in decoding the
ip-id with sequential swapped behavior.

The encoding method

optional_ip_id_lsb(behavior, indicator)
{
...
  COMPRESSED long {
    ip_id =:= irregular(16) [ 16 ];
  }
...
}

defines for the format long
that the ip-Id is transmitted irregular,
therefore as it is. Here is nothing swapped.
But in d_tcp_build_ipv4_hdr() the ip-id
swaps always:

if(decoded->id_behavior == IP_ID_BEHAVIOR_SEQ_SWAP)
{
 ipv4->ip_id = rohc_hton16(swab16(decoded->id));
}

regardless whether the ip-id was tansmitted as "long" or "short".
I propose to move the seq_swapped special handling to
d_tcp_decode_bits():

diff --git a/src/decomp/d_tcp.c b/src/decomp/d_tcp.c
index 9ededdc..d28b005 100644
--- a/src/decomp/d_tcp.c
+++ b/src/decomp/d_tcp.c
@@ -1993,6 +1994,9 @@ static bool d_tcp_decode_bits(const struct rohc_decomp_ctxt *const context,
                       ip_bits->id.bits, ip_bits->id.p);
      goto error;
     }
+ if (ip_id_behavior == IP_ID_BEHAVIOR_SEQ_SWAP) {
+ ip_decoded->id = swab16(ip_decoded->id);
+ }
     rohc_decomp_debug(context, " IP-ID = 0x%04x (decoded from "
                       "%zu-bit 0x%x with p = %d)", ip_decoded->id,
                       ip_bits->id.bits_nr, ip_bits->id.bits, ip_bits->id.p);

Now the ip-id is handled after the lsb decoding, if needed.
And omit it completely here:

@@ -2815,14 +2820,7 @@ static bool d_tcp_build_ipv4_hdr(const struct rohc_decomp_ctxt *const context,
  ipv4->frag_offset = 0;
 #endif
  /* IP-ID */
- if(decoded->id_behavior == IP_ID_BEHAVIOR_SEQ_SWAP)
- {
- ipv4->ip_id = rohc_hton16(swab16(decoded->id));
- }
- else
- {
- ipv4->ip_id = rohc_hton16(decoded->id);
- }
+ ipv4->ip_id = rohc_hton16(decoded->id);
  rohc_decomp_debug(context, " %s IP-ID = 0x%04x",
                    tcp_ip_id_behavior_get_descr(decoded->id_behavior),
                    rohc_ntoh16(ipv4->ip_id));

Best,
Klaus Warnke

Klaus Warnke (k-warnke) wrote :

The last packet in the test
ipv4_tcp_swapped-ip-id
do the job.
Unfortunately it is only one packet,
therefore to test the short format here,
additional packets were needed.

Best,
Klaus Warnke

Klaus Warnke (k-warnke) wrote :

The patch is incomplete.
I did a test with more consecutive swapped ip-id's.
Then the decode fails, because the lsb rbuf update
does not take the swapped ip-id into account.
This fix it:

diff --git a/src/decomp/d_tcp.c b/src/decomp/d_tcp.c
index 9ededdc..dabfaf4 100644
--- a/src/decomp/d_tcp.c
+++ b/src/decomp/d_tcp.c
@@ -3287,7 +3285,11 @@ static void d_tcp_update_ctxt(struct rohc_decomp_ctxt *const context,

    if(is_inner)
    {
- const uint16_t ip_id_offset = ip_context->ctxt.v4.ip_id - msn;
+ const bool ip_id_swapped =
+ ip_context->ctxt.vx.ip_id_behavior == IP_ID_BEHAVIOR_SEQ_SWAP;
+ const uint16_t ip_id = ip_id_swapped ?
+ swab16(ip_context->ctxt.v4.ip_id) : ip_context->ctxt.v4.ip_id;
+ const uint16_t ip_id_offset = ip_id - msn;
     rohc_lsb_set_ref(tcp_context->ip_id_lsb_ctxt, ip_id_offset, false);
     rohc_decomp_debug(context, "innermost IP-ID offset 0x%04x is the new "
                       "reference", ip_id_offset);

Now the new offset is computed correct in the ip-id swapped case.

Best,
Klaus Warnke

Didier Barvaux (didier-barvaux) wrote :

Klaus,

You're right. Sequential swapped IP-IDs are not handled correctly. What is surprising is that I found the very same problem (and fix) during last weekend :-)

My discovery and the related fix were on one of my private development branch, so you were not able to see it. That's why I decided to push it on Github so that you may follow it if you want. Note however that I may rebase the branch at any moment, so its history may change.

The branch is there: https://github.com/didier-barvaux/rohc/commits/dev_fuzzing_decompressor
The fix for sequential swapped IP-IDs is there: https://github.com/didier-barvaux/rohc/commit/aceb2a38f58adebbea706c45465b663db0aa3efa

Regards,
Didier

Klaus Warnke (k-warnke) wrote :

Didier,

I tried the branch, but some of my unit test
stiff fail (with the decomp from branch).
Are all conformance issue committed?

Best,
Klaus Warnke

Didier Barvaux (didier-barvaux) wrote :

Klaus,

Sorry, you're right. Some of the fixes are located in another branch "dev_improve_get_captures_script" that inherits from the "dev_fuzzing_decompressor" branch and adds some more fixes. I pushed on Github too. Please find it there: https://github.com/didier-barvaux/rohc/commits/dev_improve_get_captures_script

You should be interested in revision 447e78f05c898a99517cf99b043d7d24206c6905 that fixes the SACK block encoding: https://github.com/didier-barvaux/rohc/commit/447e78f05c898a99517cf99b043d7d24206c6905

Regards,
Didier

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

Other bug subscribers