IP-ID in extension-3 not compressed as it shall be

Bug #761955 reported by Klaus Warnke on 2011-04-15
10
This bug affects 1 person
Affects Status Importance Assigned to Milestone
rohc
Status tracked in Rohc-main
1.2.x
Medium
Didier Barvaux
1.3.x
Medium
Didier Barvaux
Rohc-main
Medium
Didier Barvaux

Bug Description

The handling of IP-ID in extension 3 is wrong.
The IP-ID is transmitted as it is, but is has to be compressed.
See RFC 4815 8.2.

   When RND=RND2=0, IP-ID is compressed, i.e., expressed as an SN offset
   and byte-swapped if NBO=0. This is the case also when 16 bits of
   IP-ID is sent in Extension 3.

ToFix:

c_generic.c: code_EXT3_packet(): /* part 6 */
Replace:
id = ipv4_get_id_nbo(ip, g_context->ip_flags.info.v4.nbo);
with
id = htons((uint16_t)(g_context->ip_flags.info.v4.id_delta & 0xffff));

For ip2 the same.
id = htons((uint16_t)(g_context->ip2_flags.info.v4.id_delta & 0xffff));

d_generic.c: decode_extension3();
Near: /* decode the IP-ID if present */

replace
ipv4_set_id(&active2->ip, GET_NEXT_16_BITS(packet));
with

                if(g_context->multiple_ip)
                {
                  uint16_t id;

                  id = GET_NEXT_16_BITS(packet);
                  id = ntohs(id);
                  id = d_ip_id_decode(&g_context->ip_id2, id, 16, *sn);
                  id = htons(id);
                  ipv4_set_id(&active2->ip, id);

/* ipv4_set_id(&active2->ip, GET_NEXT_16_BITS(packet)); */
                        rohc_debugf(3, "inner IP-ID changed (0x%04x)\n",
                                    ntohs(ipv4_get_id(active2->ip)));

same for ip2

                else
                {
                  uint16_t id;

                  id = GET_NEXT_16_BITS(packet);
                  id = ntohs(id);
                  id = d_ip_id_decode(&g_context->ip_id1, id, 16, *sn);
                  id = htons(id);
                  ipv4_set_id(&active1->ip, id);

/* ipv4_set_id(&active1->ip, GET_NEXT_16_BITS(packet)); */
                        rohc_debugf(3, "outer IP-ID changed (0x%04x)\n",
                                    ntohs(ipv4_get_id(active1->ip)));

Regards, Klaus

Didier Barvaux (didier-barvaux) wrote :

Confirmed in trunk.

Changed in rohc:
assignee: nobody → Didier Barvaux (didier-barvaux)
importance: Undecided → Medium
milestone: none → 1.4.0
status: New → Confirmed
Didier Barvaux (didier-barvaux) wrote :

Confrmed in 1.2.x releases.

Didier Barvaux (didier-barvaux) wrote :

Confrmed in 1.3.x releases.

Didier Barvaux (didier-barvaux) wrote :

The bug will not be fixed in the 1.2.x branch since the fix would change the output of the library. The README file was updated to mention the existing bug. See http://bazaar.launchpad.net/~didier-barvaux/rohc/1.2.x/revision/126

summary: - IP-ID in extension-3 not compressed, but have
+ IP-ID in extension-3 not compressed as it shall be
Didier Barvaux (didier-barvaux) wrote :

The bug will not be fixed in the 1.3.x branch since the fix would change the output of the library. The README file was updated to mention the existing bug. See http://bazaar.launchpad.net/~didier-barvaux/rohc/1.3.x/revision/155

Didier Barvaux (didier-barvaux) wrote :

I read you proposal fix. You discard the IP-ID bits stored in the main part of the packet (eg. the UOR-2-ID packet contains 5 bits of IP-ID) and use only the 16 bits of IP-ID stored in the extension 3. I did not find anything in RFCs that describes the expected behaviour (discard bits of the main part and use only bits from the extension OR split IP-ID bits into the 2 fields). Do you have more information?

Klaus Warnke (k-warnke) wrote :

Hello Didier,

I don't exactly understand what you mean, sorry.

Maybe the fix is wrong, because I don't understand the whole source
code. This is more a proposal, but it works for me (I can decompress
packets with 16bit IP-ID in extension 3 now). I looked around where
the IP-ID was handled in other cases (compressed) and copied...

To transmit the IP-ID in extension-3 uncompressed is wrong, even it is
possible, because the field is 16bit is size.

See RFC3095 page 85:
IP-ID: See the beginning of section 5.7.

And there: page 75.
IP-ID: A compressed IP-ID field

And finally 4.5.5. Offset IP-ID encoding.

Here the IP-ID is transmitted uncompressed:

/* always transmit IP-ID in Network Byte Order */
id = ipv4_get_id_nbo(ip, g_context->ip_flags.info.v4.nbo);

This is the place, where the extension-3 is handled, I hope.

Could you agree?

The fix should send/recv the IP-ID compressed instead.
The 16bit IP-ID field in extension-3 only (Part-6).

br, Klaus

Didier Barvaux (didier-barvaux) wrote :

I agree on the problem. The IP-ID value shall be stored compressed in extension 3.

I however have a question about the way one shall store the IP-ID value in a UOR-2-ID packet with extension 3. Such a packet can store 5 bits of IP-ID in its main part and 16 bits of IP-ID in extension 3. Shall be use both fields (= using a 21-bit field) ? Shall we ignore the 5-bit field when we use the 16-bit one?

Your correction stores IP-ID bits in both the 5-bit field and the 16-bit field at the compressor but uses only the 16-bit field at the decompressor.

I didn't find anything about the correct way to deal with that case in RFCs. Did you find anything on your side?

Klaus Warnke (k-warnke) wrote :

The IP-ID is stored complete in the 16bit in extension 3.
Crystal clear (haha) from rfc3095 4.5.7:

4.5.7. Encoded values across several fields in compressed headers

   When a compressed header has an extension, pieces of an encoded value
   can be present in more than one field. When an encoded value is
   split over several fields in this manner, the more significant bits
   of the value are closer to the beginning of the header. If the
   number of bits available in compressed header fields exceeds the
   number of bits in the value, the most significant field is padded
   with zeroes in its most significant bits.

The IP-ID needs only 16bit, therefore the upper 5bits are padded with zero,
if the compressed IP-ID needs more than 5bits to transmit. If 5bit are sufficient,
it should be stored in the base header.

http://www.ietf.org/mail-archive/web/rohc/current/msg01677.html

Therefore my correction is incomplete, because the 5bit in base header
should be padded with zero at compressor.

regards, Klaus

Klaus Warnke (k-warnke) wrote :

1.)
In c_generic.c line 4909.
I think the "else if" in the middle

                         /* part 2: 5 bits of IP-ID */
                          if(nr_ip_id_bits <= 5)
                                  *f_byte |= g_context->ip_flags.info.v4.id_delta & 0x1f;
                          else if(nr_ts_bits <= 13)
  => *f_byte |= (g_context->ip_flags.info.v4.id_delta >> 8) & 0x1f;
                          else
                                  *f_byte |= 0;

should be removed. If I'm in extension 3 and nr_ip_id_bits are >5, I must put the whole
IP-ID into extension-3 and the 5 bits here are left blank.
I'm unsure this is a new bug, therefore I added it here.

2.)
From my point of view here the (possible) handling of ipid2 is completely missed.
See rfc3095 page 75:

   IP-ID: A compressed IP-ID field.

      IP-ID fields in compressed base headers carry the compressed IP-ID
      of the innermost IPv4 header whose corresponding RND flag is not
      1. The rules below assume that the IP-ID is for the innermost IP
      header. If it is for an outer IP header, the RND2 and NBO2 flags
      are used instead of RND and NBO.

Here it is also possible to use ip-id 2, if ip-id is random or a ipv6 header
was there.

Klaus Warnke (k-warnke) wrote :

This fix send the ip-id compressed in extension-3.

Klaus Warnke (k-warnke) wrote :

--- src/comp/c_generic.c 2010-08-22 14:00:49 +0000
+++ src/comp/c_generic.c 2011-05-11 15:43:40 +0000
@@ -5234,8 +5240,6 @@
                        /* part 2: 5 bits of IP-ID */
                        if(nr_ip_id_bits <= 5)
                                *f_byte |= g_context->ip_flags.info.v4.id_delta & 0x1f;
- else if(nr_ts_bits <= 13)
- *f_byte |= (g_context->ip_flags.info.v4.id_delta >> 8) & 0x1f;
                        else
                                *f_byte |= 0;

sorry, forgot.

Fix committed in trunk. See http://bazaar.launchpad.net/~didier-barvaux/rohc/main/revision/229 . The commit is not atomic, it changes many more things I did at the same time. My bad.

Thank you Klaus for reporting the problem, providing a solution and answering my questions :-)

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

Other bug subscribers