rohc team mailing list archive
-
rohc team
-
Mailing list archive
-
Message #01714
Re: rohc-lib 1.7.0 - problem with new return values (rohc_status_t) and lost tcp packets
Hi Sean,
> I have discovered a problem with the new positive values for
> rohc_status_t. In the d_tcp_decode function(in d_tcp.c) the following
> code operates incorrectly due to the positive return value if there
> is an error.
>
> if(ret >= 0)
> {
> uncomp_packet->len += ret;
> ret = ROHC_STATUS_OK;
> }
> rohc_decomp_debug(context, "return %d", ret);
>
> The new return values defined in rohc_status_t are all positive so
> the code above assumes that the length of the decompression was
> returned. I assume changing the values to negative in the definition
> of rohc_status_t will fix this? or are the situations were the type
> of the return value is unsigned?
You're right! Something is wrong there. I forgot to change the called
functions and the return code check to handle the new positive return
codes. Good catch, thanks!
I use positive return code because they are now grouped in one enum
named rohc_status_t. Enum and negative values seems to cause problems
of portability, so I defined new positive return codes. For backward
compatibility, the old API functions still use the old return codes,
but internally the library shall use the new codes.
I have just written a small fix for the problem (see attachement). Tell
me if it fix the problem for you. It did with a small test case for me.
If it works, I'll commit it with the small test case. I'll also
proof-read the whole TCP profile to catch potential other similar
mistakes.
> I found this problem when implementing a fix to an assertion error in
> the decompressor when tcp compressed packets are lost. My quick fix
> to this is to return an error when the MSN increments more than 1,
> which is in the code below (from d_tcp.c).
>
> // Check the MSN received with MSN required
> if( ( (tcp_context->msn + 1) & 0x000F ) != msn)
> {
> rohc_decomp_debug(context, "last_msn = 0x%04x, waiting for msn =
> 0x%x, " "received 0x%x!", tcp_context->msn,
> (tcp_context->msn + 1) & 0x000F, msn);
> // To be completed !!!
> // Store and rework packets
> //Fixes missing packet assert problems.
> rohc_decomp_debug(context, "Error packet missing - assuming context
> damaged\n");
> goto error;
>
> }
Does the assertion looks like the below?
rfc4996.c:438: d_ip_id_lsb: Assertion `ip_id_offset == value' failed.
If yes, the assertion seems wrong. I've a work-in-progress patch to
better handle IP-ID compression/decompression in the TCP profile, but
it was not ready in time for the 1.7.0 release.
> From the comments in the code it appears that the implementation of a
> recovery mechanism is planned when packets are lost. Is an
> implementation being worked on?
It is implemented in 1.7.0 but "only" for the IP-only, IP/UDP,
IP/UDP-Lite, IP/ESP and IP/UDP/RTP profiles. The TCP profile doesn't
handle it yet. For an example of use, see option --repair of the test
tool test/robustness/lost_packet/test_lost_packet.c
Regards,
Didier
=== modified file 'src/decomp/d_tcp.c'
--- src/decomp/d_tcp.c 2014-06-21 10:59:41 +0000
+++ src/decomp/d_tcp.c 2014-06-25 07:03:48 +0000
@@ -345,12 +345,14 @@ static int d_tcp_decode_ir(struct rohc_d
const size_t rohc_length,
const size_t add_cid_len,
const size_t large_cid_len,
- unsigned char *dest);
+ unsigned char *dest,
+ size_t *const uncomp_len);
static int d_tcp_decode_irdyn(struct rohc_decomp_ctxt *context,
const unsigned char *const rohc_packet,
const size_t rohc_length,
const size_t large_cid_len,
- unsigned char *dest);
+ unsigned char *dest,
+ size_t *const uncomp_len);
static int d_tcp_decode_CO(struct rohc_decomp *decomp,
struct rohc_decomp_ctxt *context,
const unsigned char *const rohc_packet,
@@ -358,7 +360,8 @@ static int d_tcp_decode_CO(struct rohc_d
const size_t add_cid_len,
const size_t large_cid_len,
const rohc_packet_t packet_type,
- unsigned char *dest);
+ unsigned char *dest,
+ size_t *const uncomp_len);
static bool rohc_decomp_tcp_decode_seq(const struct rohc_decomp_ctxt *const context,
const uint32_t seq_bits,
@@ -763,7 +766,8 @@ static rohc_status_t d_tcp_decode(struct
{
struct d_generic_context *g_context = context->specific;
struct d_tcp_context *tcp_context = g_context->specific;
- int ret;
+ size_t uncomp_len;
+ rohc_status_t parse_ret;
size_t i;
assert(add_cid_len == 0 || add_cid_len == 1);
@@ -786,57 +790,60 @@ static rohc_status_t d_tcp_decode(struct
if((*packet_type) == ROHC_PACKET_IR)
{
/* decode IR packet */
- ret = d_tcp_decode_ir(context, rohc_buf_data(rohc_packet),
- rohc_packet.len, add_cid_len, large_cid_len,
- rohc_buf_data(*uncomp_packet));
+ parse_ret = d_tcp_decode_ir(context, rohc_buf_data(rohc_packet),
+ rohc_packet.len, add_cid_len, large_cid_len,
+ rohc_buf_data(*uncomp_packet), &uncomp_len);
}
else if((*packet_type) == ROHC_PACKET_IR_DYN)
{
/* decode IR-DYN packet */
- ret = d_tcp_decode_irdyn(context, rohc_buf_data(rohc_packet),
- rohc_packet.len, large_cid_len,
- rohc_buf_data(*uncomp_packet));
+ parse_ret = d_tcp_decode_irdyn(context, rohc_buf_data(rohc_packet),
+ rohc_packet.len, large_cid_len,
+ rohc_buf_data(*uncomp_packet),
+ &uncomp_len);
}
else
{
/* decode CO packet */
- ret = d_tcp_decode_CO(decomp, context, rohc_buf_data(rohc_packet),
- rohc_packet.len, add_cid_len, large_cid_len,
- *packet_type, rohc_buf_data(*uncomp_packet));
+ parse_ret = d_tcp_decode_CO(decomp, context, rohc_buf_data(rohc_packet),
+ rohc_packet.len, add_cid_len, large_cid_len,
+ *packet_type, rohc_buf_data(*uncomp_packet),
+ &uncomp_len);
}
/* TODO: the decode callback shall now return ROHC_OK in case of success
* instead of the uncompressed length, one shall refactor the function in
* consequence */
- if(ret >= 0)
+ if(parse_ret == ROHC_STATUS_OK)
{
- uncomp_packet->len += ret;
- ret = ROHC_STATUS_OK;
+ uncomp_packet->len += uncomp_len;
}
- rohc_decomp_debug(context, "return %d", ret);
- return ret;
+ rohc_decomp_debug(context, "return %d", parse_ret);
+ return parse_ret;
}
/**
* @brief Decode one IR packet for the TCP profile.
*
- * @param context The decompression context
- * @param rohc_packet The ROHC packet to decode
- * @param rohc_length The length of the ROHC packet to decode
- * @param add_cid_len The length of the optional Add-CID field
- * @param large_cid_len The length of the optional large CID field
- * @param dest The decoded IP packet
- * @return The length of the uncompressed IP packet
- * or ROHC_OK_NO_DATA if packet is feedback only
- * or ROHC_ERROR if an error occurs
+ * @param context The decompression context
+ * @param rohc_packet The ROHC packet to decode
+ * @param rohc_length The length of the ROHC packet to decode
+ * @param add_cid_len The length of the optional Add-CID field
+ * @param large_cid_len The length of the optional large CID field
+ * @param dest The decoded IP packet
+ * @param[out] uncomp_len The length of the uncompressed IP packet
+ * @return ROHC_STATUS_OK if decompression was successful,
+ * ROHC_ERROR_CRC if a CRC failure occured,
+ * ROHC_ERROR if another error occured
*/
static int d_tcp_decode_ir(struct rohc_decomp_ctxt *context,
const unsigned char *const rohc_packet,
const size_t rohc_length,
const size_t add_cid_len,
const size_t large_cid_len,
- unsigned char *dest)
+ unsigned char *dest,
+ size_t *const uncomp_len)
{
struct d_generic_context *g_context = context->specific;
struct d_tcp_context *tcp_context = g_context->specific;
@@ -846,7 +853,6 @@ static int d_tcp_decode_ir(struct rohc_d
unsigned int payload_size;
const uint8_t *remain_data;
size_t remain_len;
- size_t uncomp_len;
uint8_t protocol;
uint16_t size;
int read;
@@ -1090,13 +1096,13 @@ static int d_tcp_decode_ir(struct rohc_d
ip_context.uint8 = tcp_context->ip_context;
/* compute payload lengths and checksums for all headers */
- uncomp_len = size;
+ *uncomp_len = size;
do
{
if(base_header.ipvx->version == IPV4)
{
protocol = base_header.ipv4->protocol;
- base_header.ipv4->length = rohc_hton16(uncomp_len);
+ base_header.ipv4->length = rohc_hton16(*uncomp_len);
base_header.ipv4->checksum = 0;
base_header.ipv4->checksum =
ip_fast_csum(base_header.uint8,
@@ -1106,20 +1112,20 @@ static int d_tcp_decode_ir(struct rohc_d
base_header.ipv4->header_length * sizeof(uint32_t));
++base_header.ipv4;
++ip_context.v4;
- uncomp_len -= sizeof(base_header_ip_v4_t);
+ *uncomp_len -= sizeof(base_header_ip_v4_t);
}
else
{
protocol = base_header.ipv6->next_header;
- uncomp_len -= sizeof(base_header_ip_v6_t);
- base_header.ipv6->payload_length = rohc_hton16(uncomp_len);
+ *uncomp_len -= sizeof(base_header_ip_v6_t);
+ base_header.ipv6->payload_length = rohc_hton16(*uncomp_len);
rohc_decomp_debug(context, "IPv6 payload length = %d",
rohc_ntoh16(base_header.ipv6->payload_length));
++base_header.ipv6;
++ip_context.v6;
while(rohc_is_ipv6_opt(protocol))
{
- uncomp_len -= ip_context.v6_option->option_length;
+ *uncomp_len -= ip_context.v6_option->option_length;
protocol = base_header.ipv6_opt->next_header;
base_header.uint8 += ip_context.v6_option->option_length;
ip_context.uint8 += ip_context.v6_option->context_length;
@@ -1153,7 +1159,8 @@ static int d_tcp_decode_ir(struct rohc_d
context->state = ROHC_DECOMP_STATE_FC;
rohc_decomp_debug(context, "return %d", size);
- return size;
+ *uncomp_len = size;
+ return ROHC_STATUS_OK;
error:
return ROHC_STATUS_ERROR;
@@ -1163,20 +1170,22 @@ error:
/**
* @brief Decode one IR-DYN packet for the TCP profile.
*
- * @param context The decompression context
- * @param rohc_packet The ROHC packet to decode
- * @param rohc_length The length of the ROHC packet to decode
- * @param large_cid_len The length of the optional large CID field
- * @param dest The decoded IP packet
- * @return The length of the uncompressed IP packet
- * or ROHC_OK_NO_DATA if packet is feedback only
- * or ROHC_ERROR if an error occurs
+ * @param context The decompression context
+ * @param rohc_packet The ROHC packet to decode
+ * @param rohc_length The length of the ROHC packet to decode
+ * @param large_cid_len The length of the optional large CID field
+ * @param dest The decoded IP packet
+ * @param[out] uncomp_len The length of the uncompressed IP packet
+ * @return ROHC_STATUS_OK if decompression was successful,
+ * ROHC_ERROR_CRC if a CRC failure occured,
+ * ROHC_ERROR if another error occured
*/
static int d_tcp_decode_irdyn(struct rohc_decomp_ctxt *context,
const unsigned char *const rohc_packet,
const size_t rohc_length,
const size_t large_cid_len,
- unsigned char *dest)
+ unsigned char *dest,
+ size_t *const uncomp_len)
{
struct d_generic_context *g_context = context->specific;
struct d_tcp_context *tcp_context = g_context->specific;
@@ -1186,7 +1195,6 @@ static int d_tcp_decode_irdyn(struct roh
unsigned int payload_size;
const uint8_t *remain_data;
size_t remain_len;
- size_t uncomp_len;
uint8_t protocol;
uint16_t size;
int read;
@@ -1330,13 +1338,13 @@ static int d_tcp_decode_irdyn(struct roh
ip_context.uint8 = tcp_context->ip_context;
/* compute payload lengths and checksums for all headers */
- uncomp_len = size;
+ *uncomp_len = size;
do
{
if(ip_context.vx->version == IPV4)
{
protocol = ip_context.v4->protocol;
- base_header.ipv4->length = rohc_hton16(uncomp_len);
+ base_header.ipv4->length = rohc_hton16(*uncomp_len);
base_header.ipv4->checksum = 0;
base_header.ipv4->checksum =
ip_fast_csum(base_header.uint8, base_header.ipv4->header_length);
@@ -1345,20 +1353,20 @@ static int d_tcp_decode_irdyn(struct roh
base_header.ipv4->header_length);
++base_header.ipv4;
++ip_context.v4;
- uncomp_len -= sizeof(base_header_ip_v4_t);
+ *uncomp_len -= sizeof(base_header_ip_v4_t);
}
else
{
protocol = ip_context.v6->next_header;
- uncomp_len -= sizeof(base_header_ip_v6_t);
- base_header.ipv6->payload_length = rohc_hton16(uncomp_len);
+ *uncomp_len -= sizeof(base_header_ip_v6_t);
+ base_header.ipv6->payload_length = rohc_hton16(*uncomp_len);
rohc_decomp_debug(context, "payload_length = %d",
rohc_ntoh16(base_header.ipv6->payload_length));
++base_header.ipv6;
++ip_context.v6;
while(rohc_is_ipv6_opt(protocol))
{
- uncomp_len -= ip_context.v6_option->option_length;
+ *uncomp_len -= ip_context.v6_option->option_length;
protocol = ip_context.v6_option->next_header;
base_header.uint8 += ip_context.v6_option->option_length;
ip_context.uint8 += ip_context.v6_option->context_length;
@@ -1392,7 +1400,8 @@ static int d_tcp_decode_irdyn(struct roh
rohc_decomp_debug(context, "ACK number 0x%08x is the new reference",
rohc_ntoh32(tcp->ack_num));
- return size;
+ *uncomp_len = size;
+ return ROHC_STATUS_OK;
error:
return ROHC_STATUS_ERROR;
@@ -4474,17 +4483,18 @@ error:
\endverbatim
*
- * @param decomp The ROHC decompressor
- * @param context The decompression context
- * @param rohc_packet The ROHC packet to decode
- * @param rohc_length The length of the ROHC packet
- * @param add_cid_len The length of the optional Add-CID field
- * @param large_cid_len The length of the optional large CID field
- * @param packet_type The type of the ROHC packet to parse
- * @param dest OUT: The decoded IP packet
- * @return The length of the uncompressed IP packet
- * or ROHC_ERROR if an error occurs
- * or ROHC_ERROR_CRC if a CRC error occurs
+ * @param decomp The ROHC decompressor
+ * @param context The decompression context
+ * @param rohc_packet The ROHC packet to decode
+ * @param rohc_length The length of the ROHC packet
+ * @param add_cid_len The length of the optional Add-CID field
+ * @param large_cid_len The length of the optional large CID field
+ * @param packet_type The type of the ROHC packet to parse
+ * @param dest OUT: The decoded IP packet
+ * @param[out] uncomp_len The length of the uncompressed IP packet
+ * @return ROHC_STATUS_OK if decompression was successful,
+ * ROHC_ERROR_CRC if a CRC failure occured,
+ * ROHC_ERROR if another error occured
*/
static int d_tcp_decode_CO(struct rohc_decomp *decomp,
struct rohc_decomp_ctxt *context,
@@ -4493,7 +4503,8 @@ static int d_tcp_decode_CO(struct rohc_d
const size_t add_cid_len,
const size_t large_cid_len,
const rohc_packet_t packet_type,
- unsigned char *dest)
+ unsigned char *const dest,
+ size_t *const uncomp_len)
{
unsigned char *packed_rohc_packet = malloc(5000); // TODO: change that
struct d_generic_context *g_context;
@@ -6068,7 +6079,8 @@ static int d_tcp_decode_CO(struct rohc_d
#endif
free(packed_rohc_packet);
- return (uncomp_header_len + payload_len);
+ *uncomp_len = (uncomp_header_len + payload_len);
+ return ROHC_STATUS_OK;
error:
free(packed_rohc_packet);
Attachment:
signature.asc
Description: PGP signature
Follow ups
References
