[PATCH] openbsc[master]: sndcp: Allow empty SNDCP-XID indications

This is merely a historical archive of years 2008-2021, before the migration to mailman3.

A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.

dexter gerrit-no-reply at lists.osmocom.org
Fri Dec 23 10:19:27 UTC 2016


Hello Harald Welte, Jenkins Builder,

I'd like you to reexamine a change.  Please visit

    https://gerrit.osmocom.org/1487

to look at the new patch set (#3).

sndcp: Allow empty SNDCP-XID indications

In some rare cases the modem might send a xid indication that does
not contain anything except the version number field. The sgsn
ignors such SNDCP-XID indications by stripping the entire field
from the response. We found a modem in the wild that started to
act problematic when the empty SNDCP-XID was missing in the
response. This patch changes the XID negotiation behaviour in
a way that if a modem should send empty SNDCP-XID indications,
the reply will also contain an empty SNDCP-XID indication. Apart
from that the SNDCP-XID version number is now parsed and echoed
in the response. This ensures that we always reply with the version
number that the modem expects. (The version was 0 in all cases we
observed so far)

Change-Id: I097a770cb4907418f53e620a051ebb8cd110c5f2
Related: OS#1794
---
M openbsc/include/openbsc/gprs_sndcp_xid.h
M openbsc/src/gprs/gprs_sndcp.c
M openbsc/src/gprs/gprs_sndcp_xid.c
M openbsc/tests/sndcp_xid/sndcp_xid_test.c
4 files changed, 61 insertions(+), 42 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/openbsc refs/changes/87/1487/3

diff --git a/openbsc/include/openbsc/gprs_sndcp_xid.h b/openbsc/include/openbsc/gprs_sndcp_xid.h
index 02904a7..e64bc52 100644
--- a/openbsc/include/openbsc/gprs_sndcp_xid.h
+++ b/openbsc/include/openbsc/gprs_sndcp_xid.h
@@ -24,7 +24,7 @@
 #include <stdint.h>
 #include <osmocom/core/linuxlist.h>
 
-#define CURRENT_SNDCP_VERSION 0	/* See 3GPP TS 44.065, clause 8 */
+#define DEFAULT_SNDCP_VERSION 0	/* See 3GPP TS 44.065, clause 8 */
 #define MAX_ENTITIES 32		/* 3GPP TS 44.065 reserves 5 bit
 				 * for compression enitity number */
 
@@ -197,13 +197,15 @@
 
 /* Transform a list with compression fields into an SNDCP-XID message (dst) */
 int gprs_sndcp_compile_xid(uint8_t *dst, unsigned int dst_maxlen,
-			  	const struct llist_head *comp_fields);
+			   const struct llist_head *comp_fields, int version);
 
 /* Transform an SNDCP-XID message (src) into a list of SNDCP-XID fields */
-struct llist_head *gprs_sndcp_parse_xid(const void *ctx,
-				const uint8_t * src,
-				unsigned int src_len,
-				const struct llist_head *comp_fields_req);
+struct llist_head *gprs_sndcp_parse_xid(int *version,
+					const void *ctx,
+					const uint8_t *src,
+					unsigned int src_len,
+					const struct llist_head
+					*comp_fields_req);
 
 /* Find out to which compression class the specified comp-field belongs
  * (header compression or data compression?) */
diff --git a/openbsc/src/gprs/gprs_sndcp.c b/openbsc/src/gprs/gprs_sndcp.c
index 1cbeede..60455b5 100644
--- a/openbsc/src/gprs/gprs_sndcp.c
+++ b/openbsc/src/gprs/gprs_sndcp.c
@@ -960,8 +960,13 @@
 		llist_add(&v42bis_comp_field.list, &comp_fields);
 	}
 
+	/* Do not attempt to compile anything if there is no data in the list */
+	if (llist_empty(&comp_fields))
+		return 0;
+
 	/* Compile bytestream */
-	return gprs_sndcp_compile_xid(bytes, bytes_len, &comp_fields);
+	return gprs_sndcp_compile_xid(bytes, bytes_len, &comp_fields,
+				      DEFAULT_SNDCP_VERSION);
 }
 
 /* Set of SNDCP-XID bnegotiation (See also: TS 144 065,
@@ -1106,6 +1111,7 @@
 
 	int rc;
 	int compclass;
+	int version;
 
 	struct llist_head *comp_fields;
 	struct gprs_sndcp_comp_field *comp_field;
@@ -1115,21 +1121,12 @@
 	OSMO_ASSERT(lle);
 
 	/* Parse SNDCP-CID XID-Field */
-	comp_fields = gprs_sndcp_parse_xid(lle->llme,
+	comp_fields = gprs_sndcp_parse_xid(&version, lle->llme,
 					   xid_field_indication->data,
 					   xid_field_indication->data_len,
 					   NULL);
 	if (!comp_fields)
 		return -EINVAL;
-
-	/* Don't bother with empty indications */
-	if (llist_empty(comp_fields)) {
-		xid_field_response->data = NULL;
-		xid_field_response->data_len = 0;
-		DEBUGP(DSNDCP,
-		       "SNDCP-XID indication did not contain any parameters!\n");
-		return 0;
-	}
 
 	/* Handle compression entites */
 	DEBUGP(DSNDCP, "SNDCP-XID-IND (ms):\n");
@@ -1168,7 +1165,7 @@
 	/* Compile modified SNDCP-XID bytes */
 	rc = gprs_sndcp_compile_xid(xid_field_response->data,
 				    xid_field_indication->data_len,
-				    comp_fields);
+				    comp_fields, 0);
 
 	if (rc > 0)
 		xid_field_response->data_len = rc;
@@ -1210,7 +1207,7 @@
 	OSMO_ASSERT(xid_field_request);
 
 	/* Parse SNDCP-CID XID-Field */
-	comp_fields_req = gprs_sndcp_parse_xid(lle->llme,
+	comp_fields_req = gprs_sndcp_parse_xid(NULL, lle->llme,
 					       xid_field_request->data,
 					       xid_field_request->data_len,
 					       NULL);
@@ -1221,7 +1218,7 @@
 	gprs_sndcp_dump_comp_fields(comp_fields_req, LOGL_DEBUG);
 
 	/* Parse SNDCP-CID XID-Field */
-	comp_fields_conf = gprs_sndcp_parse_xid(lle->llme,
+	comp_fields_conf = gprs_sndcp_parse_xid(NULL, lle->llme,
 						xid_field_conf->data,
 						xid_field_conf->data_len,
 						comp_fields_req);
diff --git a/openbsc/src/gprs/gprs_sndcp_xid.c b/openbsc/src/gprs/gprs_sndcp_xid.c
index bb43eab..dfea5fe 100644
--- a/openbsc/src/gprs/gprs_sndcp_xid.c
+++ b/openbsc/src/gprs/gprs_sndcp_xid.c
@@ -549,26 +549,29 @@
 
 /* Transform a list with compression fields into an SNDCP-XID message (dst) */
 int gprs_sndcp_compile_xid(uint8_t *dst, unsigned int dst_maxlen,
-			   const struct llist_head *comp_fields)
+			   const struct llist_head *comp_fields, int version)
 {
 	int rc;
 	int byte_counter = 0;
 	uint8_t comp_bytes[512];
-	uint8_t xid_version_number[1] = { CURRENT_SNDCP_VERSION };
+	uint8_t xid_version_number[1];
 
 	OSMO_ASSERT(comp_fields);
 	OSMO_ASSERT(dst);
 	OSMO_ASSERT(dst_maxlen >= 2 + sizeof(xid_version_number));
 
-	/* Bail if there is no input */
-	if (llist_empty(comp_fields))
-		return -EINVAL;
+	/* Prepend header with version number */
+	if (version >= 0) {
+		xid_version_number[0] = (uint8_t) (version & 0xff);
+		dst =
+		    tlv_put(dst, SNDCP_XID_VERSION_NUMBER,
+			    sizeof(xid_version_number), xid_version_number);
+		byte_counter += (sizeof(xid_version_number) + 2);
+	}
 
-	/* Prepend header */
-	dst =
-	    tlv_put(dst, SNDCP_XID_VERSION_NUMBER,
-		    sizeof(xid_version_number), xid_version_number);
-	byte_counter += (sizeof(xid_version_number) + 2);
+	/* Stop if there is no compression fields supplied */
+	if (llist_empty(comp_fields))
+		return byte_counter;
 
 	/* Add data compression fields */
 	rc = gprs_sndcp_pack_fields(comp_fields, comp_bytes,
@@ -1283,11 +1286,10 @@
 }
 
 /* Transform an SNDCP-XID message (src) into a list of SNDCP-XID fields */
-static int gprs_sndcp_decode_xid(struct llist_head *comp_fields,
+static int gprs_sndcp_decode_xid(int *version, struct llist_head *comp_fields,
 				 const uint8_t *src, unsigned int src_len,
-				 const struct
-				 entity_algo_table
-				 *lt, unsigned int lt_len)
+				 const struct entity_algo_table *lt,
+				 unsigned int lt_len)
 {
 	int src_pos = 0;
 	uint8_t tag;
@@ -1296,6 +1298,10 @@
 	int byte_counter = 0;
 	int rc;
 	int tlv_count = 0;
+
+	/* Preset version value as invalid */
+	if (version)
+		*version = -1;
 
 	/* Valid TLV-Tag and types */
 	static const struct tlv_definition sndcp_xid_def = {
@@ -1326,6 +1332,10 @@
 			talloc_free(comp_fields);
 			return -EINVAL;
 		}
+
+		/* Decode sndcp xid version number */
+		if (version && tag == SNDCP_XID_VERSION_NUMBER)
+			*version = val[0];
 
 		/* Decode compression parameters */
 		if ((tag == SNDCP_XID_PROTOCOL_COMPRESSION)
@@ -1548,7 +1558,8 @@
 }
 
 /* Transform an SNDCP-XID message (src) into a list of SNDCP-XID fields */
-struct llist_head *gprs_sndcp_parse_xid(const void *ctx,
+struct llist_head *gprs_sndcp_parse_xid(int *version,
+					const void *ctx,
 					const uint8_t *src,
 					unsigned int src_len,
 					const struct llist_head
@@ -1559,6 +1570,12 @@
 	struct llist_head *comp_fields;
 	struct entity_algo_table lt[MAX_ENTITIES * 2];
 
+	/* In case of a zero length field, just exit */
+	if (src_len == 0)
+		return NULL;
+
+	/* We should go any further if we have a field length greater
+	 * zero and a null pointer as buffer! */
 	OSMO_ASSERT(src);
 
 	comp_fields = talloc_zero(ctx, struct llist_head);
@@ -1575,8 +1592,8 @@
 		}
 
 		/* Parse SNDCP-CID XID-Field */
-		rc = gprs_sndcp_decode_xid(comp_fields, src, src_len, lt,
-					   lt_len);
+		rc = gprs_sndcp_decode_xid(version, comp_fields, src, src_len,
+					   lt, lt_len);
 		if (rc < 0) {
 			talloc_free(comp_fields);
 			return NULL;
@@ -1591,7 +1608,8 @@
 
 	} else {
 		/* Parse SNDCP-CID XID-Field */
-		rc = gprs_sndcp_decode_xid(comp_fields, src, src_len, NULL, 0);
+		rc = gprs_sndcp_decode_xid(version, comp_fields, src, src_len,
+					   NULL, 0);
 		if (rc < 0) {
 			talloc_free(comp_fields);
 			return NULL;
diff --git a/openbsc/tests/sndcp_xid/sndcp_xid_test.c b/openbsc/tests/sndcp_xid/sndcp_xid_test.c
index 3a33619..151dd2b 100644
--- a/openbsc/tests/sndcp_xid/sndcp_xid_test.c
+++ b/openbsc/tests/sndcp_xid/sndcp_xid_test.c
@@ -47,13 +47,14 @@
 	uint8_t xid_r[512];
 
 	/* Parse and show contained comp fields */
-	comp_fields = gprs_sndcp_parse_xid(ctx, xid, sizeof(xid), NULL);
+	comp_fields = gprs_sndcp_parse_xid(NULL, ctx, xid, sizeof(xid), NULL);
 	OSMO_ASSERT(comp_fields);
 	printf("Decoded:\n");
 	gprs_sndcp_dump_comp_fields(comp_fields, DSNDCP);
 
 	/* Encode comp-fields again */
-	rc = gprs_sndcp_compile_xid(xid_r,sizeof(xid_r), comp_fields);
+	rc = gprs_sndcp_compile_xid(xid_r,sizeof(xid_r), comp_fields,
+				    DEFAULT_SNDCP_VERSION);
 	printf("Result length=%i\n",rc);
 	printf("Encoded:  %s\n", osmo_hexdump_nospc(xid, sizeof(xid)));
 	printf("Rencoded: %s\n", osmo_hexdump_nospc(xid_r, rc));
@@ -226,13 +227,14 @@
 	gprs_sndcp_dump_comp_fields(&comp_fields, DSNDCP);
 
 	/* Encode SNDCP-XID fields */
-	rc = gprs_sndcp_compile_xid(xid, xid_len, &comp_fields);
+	rc = gprs_sndcp_compile_xid(xid, xid_len, &comp_fields,
+				    DEFAULT_SNDCP_VERSION);
 	OSMO_ASSERT(rc > 0);
 
 	printf("Encoded:  %s (%i bytes)\n", osmo_hexdump_nospc(xid, rc), rc);
 
 	/* Parse and show contained comp fields */
-	comp_fields_dec = gprs_sndcp_parse_xid(ctx, xid, rc, NULL);
+	comp_fields_dec = gprs_sndcp_parse_xid(NULL, ctx, xid, rc, NULL);
 	OSMO_ASSERT(comp_fields_dec);
 
 	printf("Decoded:\n");

-- 
To view, visit https://gerrit.osmocom.org/1487
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I097a770cb4907418f53e620a051ebb8cd110c5f2
Gerrit-PatchSet: 3
Gerrit-Project: openbsc
Gerrit-Branch: master
Gerrit-Owner: Max <msuraev at sysmocom.de>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: dexter <pmaier at sysmocom.de>



More information about the gerrit-log mailing list