December 2022 - gerrit-log - lists.osmocom.org

Change in libosmocore[master]: gsm0808: add logging for some IE encoding errors

by fixeria

Attention is currently required from: neels. fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/30581 ) Change subject: gsm0808: add logging for some IE encoding errors ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: It can still be used without having to initialize logging, as we do have a fallback mechanism: see #define LOGPSRCC and logp_stub(). > category DLGLOBAL is a bad fit, should we add DLBSSAP now? I don't think it's worth it. Be it good or bad, this is the only value we can use here ATM. > a solution would be an error_cb(), concise error info is provided by the lib, the caller decides how to handle it. Too much complexity for... simply printing something that should not normally happen. IMHO. > should we also add logging and better handling for all other error cases in this file? There are still plenty of assert()s in other enc_ functions waiting to be replaced ;) -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/30581 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Idd9c490e7a2d37817004590629092c4bb6f2d758 Gerrit-Change-Number: 30581 Gerrit-PatchSet: 1 Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: neels <nhofmeyr(a)sysmocom.de> Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de> Gerrit-Comment-Date: Tue, 13 Dec 2022 22:58:10 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: neels <nhofmeyr(a)sysmocom.de> Gerrit-MessageType: comment

2 years, 6 months

1
0
0 0

Change in libosmocore[master]: gsm0808: add gsm0808_enc_speech_codec[_list]2()

by fixeria

fixeria has submitted this change. ( https://gerrit.osmocom.org/c/libosmocore/+/30574 ) Change subject: gsm0808: add gsm0808_enc_speech_codec[_list]2() ...................................................................... gsm0808: add gsm0808_enc_speech_codec[_list]2() The problem with most of the existing gsm0808_* functions in this file is that they assert() too much, assuming that their callers always pass perfectly valid input parameters. But this is impossible on practice, as there can be bugs in complex projects using them, liks osmo-bsc. It was reported by a customer that a heavily loaded osmo-bsc crashed a few times, dropping more than 100 sites without network coverage for a few minutes. As was revealed during the investigaion, it crashed due to a failing assert at the end of enc_speech_codec(): OSMO_ASSERT(sc->cfg == 0); The problem is that somehow osmo-bsc is passing an unexpected sc->cfg value to gsm0808_create_ass_compl2(), in particular 0x02, while the given sc->type value (GSM0808_SCT_HR1) implies that there cannot be any configuration bits on the wire. The reason why and under which circumstances this can be happening is not clear yet, but what we agreed on so far is that the library API should be enforcing correctness of the input parameters in a less agressive way, rather than aborting the process without letting it any chance to recover. Modify the original gsm0808_enc_speech_codec[_list]() functions, so that a negative value is returned in case of an error. Rename them and add backwards compatibility wrappers, because it's public API. A separate patch making use of the new API follows next. Change-Id: I199ffa0ba4a64813238519178155dfc767aa3975 Related: SYS#6229 --- M include/osmocom/gsm/gsm0808_utils.h M src/gsm/gsm0808_utils.c M src/gsm/libosmogsm.map 3 files changed, 61 insertions(+), 25 deletions(-) Approvals: Jenkins Builder: Verified laforge: Looks good to me, but someone else must approve neels: Looks good to me, approved diff --git a/include/osmocom/gsm/gsm0808_utils.h b/include/osmocom/gsm/gsm0808_utils.h index 6abfeec..18f3ebe 100644 --- a/include/osmocom/gsm/gsm0808_utils.h +++ b/include/osmocom/gsm/gsm0808_utils.h @@ -111,12 +111,17 @@ int gsm0808_dec_lcls(struct osmo_lcls *lcls, const struct tlv_parsed *tp); uint8_t gsm0808_enc_speech_codec(struct msgb *msg, - const struct gsm0808_speech_codec *sc); + const struct gsm0808_speech_codec *sc) + OSMO_DEPRECATED("use gsm0808_enc_speech_codec2() instead"); +int gsm0808_enc_speech_codec2(struct msgb *msg, + const struct gsm0808_speech_codec *sc); int gsm0808_dec_speech_codec(struct gsm0808_speech_codec *sc, const uint8_t *elem, uint8_t len); uint8_t gsm0808_enc_speech_codec_list(struct msgb *msg, - const struct gsm0808_speech_codec_list - *scl); + const struct gsm0808_speech_codec_list *scl) + OSMO_DEPRECATED("use gsm0808_enc_speech_codec_list2() instead"); +int gsm0808_enc_speech_codec_list2(struct msgb *msg, + const struct gsm0808_speech_codec_list *scl); int gsm0808_dec_speech_codec_list(struct gsm0808_speech_codec_list *scl, const uint8_t *elem, uint8_t len); uint8_t gsm0808_enc_channel_type(struct msgb *msg, diff --git a/src/gsm/gsm0808_utils.c b/src/gsm/gsm0808_utils.c index a1f51eb..9d787f2 100644 --- a/src/gsm/gsm0808_utils.c +++ b/src/gsm/gsm0808_utils.c @@ -209,10 +209,10 @@ gsm48_decode_lai2(&lai, decoded); } -/* Helper function for gsm0808_enc_speech_codec() - * and gsm0808_enc_speech_codec_list() */ -static uint8_t enc_speech_codec(struct msgb *msg, - const struct gsm0808_speech_codec *sc) +/* Helper function for gsm0808_enc_speech_codec[_list](). + * Returns number of bytes appended; negative on error. */ +static int enc_speech_codec(struct msgb *msg, + const struct gsm0808_speech_codec *sc) { /* See also 3GPP TS 48.008 3.2.2.103 Speech Codec List */ uint8_t header = 0; @@ -240,8 +240,7 @@ break; default: /* Invalid codec type specified */ - OSMO_ASSERT(false); - break; + return -EINVAL; } old_tail = msg->tail; @@ -277,11 +276,13 @@ case GSM0808_SCT_FR5: case GSM0808_SCT_HR4: case GSM0808_SCT_CSD: - OSMO_ASSERT((sc->cfg & 0xff00) == 0); + if (sc->cfg >> 8) + return -EINVAL; msgb_put_u8(msg, (uint8_t) sc->cfg & 0xff); break; default: - OSMO_ASSERT(sc->cfg == 0); + if (sc->cfg != 0) + return -EINVAL; break; } @@ -291,24 +292,38 @@ /*! Encode TS 08.08 Speech Codec IE * \param[out] msg Message Buffer to which IE will be appended * \param[in] sc Speech Codec to be encoded into IE - * \returns number of bytes appended to \a msg */ -uint8_t gsm0808_enc_speech_codec(struct msgb *msg, - const struct gsm0808_speech_codec *sc) + * \returns number of bytes appended to \a msg; negative on error */ +int gsm0808_enc_speech_codec2(struct msgb *msg, + const struct gsm0808_speech_codec *sc) { /*! See also 3GPP TS 48.008 3.2.2.103 Speech Codec List */ - uint8_t *old_tail; uint8_t *tlv_len; + int rc; msgb_put_u8(msg, GSM0808_IE_SPEECH_CODEC); tlv_len = msgb_put(msg, 1); - old_tail = msg->tail; - enc_speech_codec(msg, sc); + rc = enc_speech_codec(msg, sc); + if (rc < 0) + return rc; - *tlv_len = (uint8_t) (msg->tail - old_tail); + *tlv_len = rc; return *tlv_len + 2; } +/*! Deprecated: gsm0808_enc_speech_codec2() wrapper for backwards compatibility. + * Mimics the old behavior: crash on missing and/or invalid input. */ +uint8_t gsm0808_enc_speech_codec(struct msgb *msg, + const struct gsm0808_speech_codec *sc) +{ + int rc; + + rc = gsm0808_enc_speech_codec2(msg, sc); + OSMO_ASSERT(rc > 0); + + return rc; +} + /*! Decode TS 08.08 Speech Codec IE * \param[out] sc Caller-allocated memory for Speech Codec * \param[in] elem IE value to be decoded @@ -392,15 +407,14 @@ /*! Encode TS 08.08 Speech Codec list * \param[out] msg Message Buffer to which IE is to be appended * \param[in] scl Speech Codec List to be encoded into IE - * \returns number of bytes added to \a msg */ -uint8_t gsm0808_enc_speech_codec_list(struct msgb *msg, - const struct gsm0808_speech_codec_list *scl) + * \returns number of bytes added to \a msg; negative on error */ +int gsm0808_enc_speech_codec_list2(struct msgb *msg, + const struct gsm0808_speech_codec_list *scl) { /*! See also 3GPP TS 48.008 3.2.2.103 Speech Codec List */ uint8_t *old_tail; uint8_t *tlv_len; unsigned int i; - uint8_t rc; unsigned int bytes_used = 0; msgb_put_u8(msg, GSM0808_IE_SPEECH_CODEC_LIST); @@ -408,16 +422,31 @@ old_tail = msg->tail; for (i = 0; i < scl->len; i++) { - rc = enc_speech_codec(msg, &scl->codec[i]); - OSMO_ASSERT(rc >= 1); + int rc = enc_speech_codec(msg, &scl->codec[i]); + if (rc < 1) + return rc; bytes_used += rc; - OSMO_ASSERT(bytes_used <= 255); + if (bytes_used > 0xff) + return -EOVERFLOW; } *tlv_len = (uint8_t) (msg->tail - old_tail); return *tlv_len + 2; } +/*! Deprecated: gsm0808_enc_speech_codec_list2() wrapper for backwards compatibility. + * Mimics the old behavior: crash on missing and/or invalid input. */ +uint8_t gsm0808_enc_speech_codec_list(struct msgb *msg, + const struct gsm0808_speech_codec_list *scl) +{ + int rc; + + rc = gsm0808_enc_speech_codec_list2(msg, scl); + OSMO_ASSERT(rc > 0); + + return rc; +} + /*! Decode TS 08.08 Speech Codec list IE * \param[out] scl Caller-provided memory to store codec list * \param[in] elem IE value to be decoded diff --git a/src/gsm/libosmogsm.map b/src/gsm/libosmogsm.map index b971ca0..7b5cd94 100644 --- a/src/gsm/libosmogsm.map +++ b/src/gsm/libosmogsm.map @@ -212,8 +212,10 @@ gsm0808_dec_aoip_trasp_addr; gsm0808_dec_osmux_cid; gsm0808_enc_speech_codec; +gsm0808_enc_speech_codec2; gsm0808_dec_speech_codec; gsm0808_enc_speech_codec_list; +gsm0808_enc_speech_codec_list2; gsm0808_dec_speech_codec_list; gsm0808_enc_channel_type; gsm0808_dec_channel_type; -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/30574 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: I199ffa0ba4a64813238519178155dfc767aa3975 Gerrit-Change-Number: 30574 Gerrit-PatchSet: 2 Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de> Gerrit-MessageType: merged

2 years, 6 months

1
0
0 0

Change in libosmocore[master]: gsm0808: use new gsm0808_enc_speech_codec[_list]2() API

by fixeria

fixeria has submitted this change. ( https://gerrit.osmocom.org/c/libosmocore/+/30580 ) Change subject: gsm0808: use new gsm0808_enc_speech_codec[_list]2() API ...................................................................... gsm0808: use new gsm0808_enc_speech_codec[_list]2() API The motivation behind adding and using the new API is explained in the preceeding change [1]. Whenever any of the encoding functions fails to encode either a Speech Codec or a Codec List IE, free() the msgb and return NULL. Change-Id: I28219b61b9347f0652f9fd0c717f6cdf3c63e8f9 Related: [1] I199ffa0ba4a64813238519178155dfc767aa3975 Related: SYS#6229 --- M src/gsm/gsm0808.c M tests/gsm0808/gsm0808_test.c 2 files changed, 89 insertions(+), 29 deletions(-) Approvals: Jenkins Builder: Verified laforge: Looks good to me, but someone else must approve neels: Looks good to me, approved diff --git a/src/gsm/gsm0808.c b/src/gsm/gsm0808.c index 89a8874..ec4c39b 100644 --- a/src/gsm/gsm0808.c +++ b/src/gsm/gsm0808.c @@ -100,13 +100,19 @@ msgb_l3len(msg_l3), msg_l3->l3h); /* AoIP: add Codec List (BSS Supported) 3.2.2.103 */ - if (scl) - gsm0808_enc_speech_codec_list(msg, scl); + if (scl) { + if (gsm0808_enc_speech_codec_list2(msg, scl) < 0) + goto exit_free; + } /* push the bssmap header */ msg->l3h = msgb_tv_push(msg, BSSAP_MSG_BSS_MANAGEMENT, msgb_length(msg)); return msg; + +exit_free: + msgb_free(msg); + return NULL; } /*! Create "Complete L3 Info" for A, legacy implementation. @@ -530,8 +536,10 @@ } /* AoIP: Codec List (MSC Preferred) 3.2.2.103 */ - if (scl) - gsm0808_enc_speech_codec_list(msg, scl); + if (scl) { + if (gsm0808_enc_speech_codec_list2(msg, scl) < 0) + goto exit_free; + } /* AoIP: Call Identifier 3.2.2.105 */ if (ci) { @@ -552,6 +560,10 @@ msgb_tv_push(msg, BSSAP_MSG_BSS_MANAGEMENT, msgb_length(msg)); return msg; + +exit_free: + msgb_free(msg); + return NULL; } /*! Create BSSMAP Assignment Request message, 3GPP TS 48.008 §3.2.1.1. @@ -616,12 +628,16 @@ gsm0808_enc_aoip_trasp_addr(msg, ss); /* AoIP: Speech Codec (Chosen) 3.2.2.104 */ - if (sc) - gsm0808_enc_speech_codec(msg, sc); + if (sc) { + if (gsm0808_enc_speech_codec2(msg, sc) < 0) + goto exit_free; + } /* AoIP: add Codec List (BSS Supported) 3.2.2.103 */ - if (scl) - gsm0808_enc_speech_codec_list(msg, scl); + if (scl) { + if (gsm0808_enc_speech_codec_list2(msg, scl) < 0) + goto exit_free; + } /* FIXME: write LSA identifier 3.2.2.15 - see 3GPP TS 43.073 */ @@ -632,6 +648,10 @@ msg->l3h = msgb_tv_push(msg, BSSAP_MSG_BSS_MANAGEMENT, msgb_length(msg)); return msg; + +exit_free: + msgb_free(msg); + return NULL; } /*! Create BSSMAP Assignment Completed message @@ -693,13 +713,19 @@ /* Circuit pool list 3.2.2.46 */ /* AoIP: add Codec List (BSS Supported) 3.2.2.103 */ - if (scl) - gsm0808_enc_speech_codec_list(msg, scl); + if (scl) { + if (gsm0808_enc_speech_codec_list2(msg, scl) < 0) + goto exit_free; + } /* update the size */ msg->l3h = msgb_tv_push(msg, BSSAP_MSG_BSS_MANAGEMENT, msgb_length(msg)); return msg; + +exit_free: + msgb_free(msg); + return NULL; } /*! Create BSSMAP Assignment Failure message @@ -992,8 +1018,10 @@ if (params->aoip_transport_layer) gsm0808_enc_aoip_trasp_addr(msg, params->aoip_transport_layer); - if (params->codec_list_msc_preferred) - gsm0808_enc_speech_codec_list(msg, params->codec_list_msc_preferred); + if (params->codec_list_msc_preferred) { + if (gsm0808_enc_speech_codec_list2(msg, params->codec_list_msc_preferred) < 0) + goto exit_free; + } if (params->call_id_present) { uint8_t val[4]; @@ -1013,6 +1041,10 @@ msg->l3h = msgb_tv_push(msg, BSSAP_MSG_BSS_MANAGEMENT, msgb_length(msg)); return msg; + +exit_free: + msgb_free(msg); + return NULL; } /*! Create BSSMAP HANDOVER REQUEST ACKNOWLEDGE message, 3GPP TS 48.008 3.2.1.10. @@ -1048,17 +1080,25 @@ /* AoIP: add Codec List (BSS Supported) 3.2.2.103. * (codec_list_bss_supported was added to struct gsm0808_handover_request_ack later than speech_codec_chosen * below, but it needs to come before it in the message coding). */ - if (params->more_items && params->codec_list_bss_supported.len) - gsm0808_enc_speech_codec_list(msg, &params->codec_list_bss_supported); + if (params->more_items && params->codec_list_bss_supported.len) { + if (gsm0808_enc_speech_codec_list2(msg, &params->codec_list_bss_supported) < 0) + goto exit_free; + } /* AoIP: Speech Codec (Chosen) 3.2.2.104 */ - if (params->speech_codec_chosen_present) - gsm0808_enc_speech_codec(msg, &params->speech_codec_chosen); + if (params->speech_codec_chosen_present) { + if (gsm0808_enc_speech_codec2(msg, &params->speech_codec_chosen) < 0) + goto exit_free; + } /* prepend header with final length */ msg->l3h = msgb_tv_push(msg, BSSAP_MSG_BSS_MANAGEMENT, msgb_length(msg)); return msg; + +exit_free: + msgb_free(msg); + return NULL; } /*! Same as gsm0808_create_handover_request_ack2() but with less parameters. @@ -1164,12 +1204,16 @@ msgb_tlv_put(msg, GSM0808_IE_RR_CAUSE, 1, &params->rr_cause); /* AoIP: Speech Codec (Chosen) 3.2.2.104 */ - if (params->speech_codec_chosen_present) - gsm0808_enc_speech_codec(msg, &params->speech_codec_chosen); + if (params->speech_codec_chosen_present) { + if (gsm0808_enc_speech_codec2(msg, &params->speech_codec_chosen) < 0) + goto exit_free; + } /* AoIP: add Codec List (BSS Supported) 3.2.2.103 */ - if (params->codec_list_bss_supported.len) - gsm0808_enc_speech_codec_list(msg, &params->codec_list_bss_supported); + if (params->codec_list_bss_supported.len) { + if (gsm0808_enc_speech_codec_list2(msg, &params->codec_list_bss_supported) < 0) + goto exit_free; + } /* Chosen Encryption Algorithm 3.2.2.44 */ if (params->chosen_encr_alg_present && params->chosen_encr_alg > 0) @@ -1183,6 +1227,10 @@ msg->l3h = msgb_tv_push(msg, BSSAP_MSG_BSS_MANAGEMENT, msgb_length(msg)); return msg; + +exit_free: + msgb_free(msg); + return NULL; } /*! Create BSSMAP HANDOVER FAILURE message, 3GPP TS 48.008 3.2.1.16. @@ -1206,13 +1254,19 @@ msgb_tlv_put(msg, GSM0808_IE_RR_CAUSE, 1, &params->rr_cause); /* AoIP: add Codec List (BSS Supported) 3.2.2.103 */ - if (params->codec_list_bss_supported.len) - gsm0808_enc_speech_codec_list(msg, &params->codec_list_bss_supported); + if (params->codec_list_bss_supported.len) { + if (gsm0808_enc_speech_codec_list2(msg, &params->codec_list_bss_supported) < 0) + goto exit_free; + } /* prepend header with final length */ msg->l3h = msgb_tv_push(msg, BSSAP_MSG_BSS_MANAGEMENT, msgb_length(msg)); return msg; + +exit_free: + msgb_free(msg); + return NULL; } /*! Create BSSMAP HANDOVER PERFORMED message, 3GPP TS 48.008 3.2.1.25. @@ -1248,8 +1302,10 @@ msgb_tv_put(msg, GSM0808_IE_SPEECH_VERSION, params->speech_version_chosen); /* AoIP: Speech Codec (chosen) 3.2.2.104 */ - if (params->speech_codec_chosen_present) - gsm0808_enc_speech_codec(msg, &params->speech_codec_chosen); + if (params->speech_codec_chosen_present) { + if (gsm0808_enc_speech_codec2(msg, &params->speech_codec_chosen) < 0) + goto exit_free; + } /* LCLS-BSS-Status 3.2.2.119 */ if (params->lcls_bss_status_present) @@ -1259,6 +1315,10 @@ msg->l3h = msgb_tv_push(msg, BSSAP_MSG_BSS_MANAGEMENT, msgb_length(msg)); return msg; + +exit_free: + msgb_free(msg); + return NULL; } /*! Create BSSMAP COMMON ID message, 3GPP TS 48.008 3.2.1.68. diff --git a/tests/gsm0808/gsm0808_test.c b/tests/gsm0808/gsm0808_test.c index 2734f91..ab9f29b 100644 --- a/tests/gsm0808/gsm0808_test.c +++ b/tests/gsm0808/gsm0808_test.c @@ -959,7 +959,7 @@ int rc_dec; msg = msgb_alloc(1024, "output buffer"); - rc_enc = gsm0808_enc_speech_codec(msg, &enc_sc); + rc_enc = gsm0808_enc_speech_codec2(msg, &enc_sc); OSMO_ASSERT(rc_enc == 3); rc_dec = gsm0808_dec_speech_codec(&dec_sc, msg->data + 2, msg->len - 2); @@ -985,7 +985,7 @@ int rc_dec; msg = msgb_alloc(1024, "output buffer"); - rc_enc = gsm0808_enc_speech_codec(msg, &enc_sc); + rc_enc = gsm0808_enc_speech_codec2(msg, &enc_sc); OSMO_ASSERT(rc_enc == 5); rc_dec = gsm0808_dec_speech_codec(&dec_sc, msg->data + 2, msg->len - 2); @@ -1010,7 +1010,7 @@ int rc_dec; msg = msgb_alloc(1024, "output buffer"); - rc_enc = gsm0808_enc_speech_codec(msg, &enc_sc); + rc_enc = gsm0808_enc_speech_codec2(msg, &enc_sc); OSMO_ASSERT(rc_enc == 5); rc_dec = gsm0808_dec_speech_codec(&dec_sc, msg->data + 2, msg->len - 2); @@ -1053,7 +1053,7 @@ int rc_dec; msg = msgb_alloc(1024, "output buffer"); - rc_enc = gsm0808_enc_speech_codec_list(msg, &enc_scl); + rc_enc = gsm0808_enc_speech_codec_list2(msg, &enc_scl); OSMO_ASSERT(rc_enc == 9); rc_dec = gsm0808_dec_speech_codec_list(&dec_scl, msg->data + 2, msg->len - 2); @@ -1075,7 +1075,7 @@ int rc_dec; msg = msgb_alloc(1024, "output buffer"); - rc_enc = gsm0808_enc_speech_codec_list(msg, &enc_scl); + rc_enc = gsm0808_enc_speech_codec_list2(msg, &enc_scl); OSMO_ASSERT(rc_enc == 2); rc_dec = gsm0808_dec_speech_codec_list(&dec_scl, msg->data + 2, msg->len - 2); -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/30580 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: I28219b61b9347f0652f9fd0c717f6cdf3c63e8f9 Gerrit-Change-Number: 30580 Gerrit-PatchSet: 1 Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de> Gerrit-MessageType: merged

2 years, 6 months

1
0
0 0

Change in libosmocore[master]: gsm0808: remove over-defensive assert()s for function parameters

by fixeria

fixeria has submitted this change. ( https://gerrit.osmocom.org/c/libosmocore/+/30579 ) Change subject: gsm0808: remove over-defensive assert()s for function parameters ...................................................................... gsm0808: remove over-defensive assert()s for function parameters I believe the gsm0808_* API is mature enough to avoid assert()ing pointers accepted via function parameters. We can assume the caller never passes NULL, as we do in almost all public API. Change-Id: If9b4c92ace68191f5ddcc0a8a340fccbfe0f3dc0 --- M src/gsm/gsm0808_utils.c 1 file changed, 0 insertions(+), 29 deletions(-) Approvals: Jenkins Builder: Verified laforge: Looks good to me, but someone else must approve neels: Looks good to me, approved diff --git a/src/gsm/gsm0808_utils.c b/src/gsm/gsm0808_utils.c index 5b7fff2..a1f51eb 100644 --- a/src/gsm/gsm0808_utils.c +++ b/src/gsm/gsm0808_utils.c @@ -90,8 +90,6 @@ const uint8_t len_tl = 2; uint8_t len_v = sizeof(port); - OSMO_ASSERT(msg); - OSMO_ASSERT(ss); OSMO_ASSERT(ss->ss_family == AF_INET || ss->ss_family == AF_INET6); switch (ss->ss_family) { @@ -141,7 +139,6 @@ struct sockaddr_in6 sin6; const uint8_t *old_elem = elem; - OSMO_ASSERT(ss); if (!elem) return -EINVAL; if (len == 0) @@ -189,7 +186,6 @@ * \returns number of bytes parsed */ int gsm0808_dec_osmux_cid(uint8_t *cid, const uint8_t *elem, uint8_t len) { - OSMO_ASSERT(cid); if (!elem) return -EINVAL; if (len != 1) @@ -303,9 +299,6 @@ uint8_t *old_tail; uint8_t *tlv_len; - OSMO_ASSERT(msg); - OSMO_ASSERT(sc); - msgb_put_u8(msg, GSM0808_IE_SPEECH_CODEC); tlv_len = msgb_put(msg, 1); old_tail = msg->tail; @@ -328,7 +321,6 @@ uint8_t header; const uint8_t *old_elem = elem; - OSMO_ASSERT(sc); if (!elem) return -EINVAL; if (len == 0) @@ -411,9 +403,6 @@ uint8_t rc; unsigned int bytes_used = 0; - OSMO_ASSERT(msg); - OSMO_ASSERT(scl); - msgb_put_u8(msg, GSM0808_IE_SPEECH_CODEC_LIST); tlv_len = msgb_put(msg, 1); old_tail = msg->tail; @@ -443,7 +432,6 @@ int rc; uint8_t decoded = 0; - OSMO_ASSERT(scl); if (!elem) return -EINVAL; @@ -480,8 +468,6 @@ uint8_t *old_tail; uint8_t *tlv_len; - OSMO_ASSERT(msg); - OSMO_ASSERT(ct); OSMO_ASSERT(ct->perm_spch_len <= CHANNEL_TYPE_ELEMENT_MAXLEN - 2); /* FIXME: Implement encoding support for Data @@ -522,7 +508,6 @@ uint8_t byte; const uint8_t *old_elem = elem; - OSMO_ASSERT(ct); if (!elem) return -EINVAL; if (len < 3 || len > 11) @@ -711,8 +696,6 @@ uint8_t *old_tail; uint8_t *tlv_len; - OSMO_ASSERT(msg); - OSMO_ASSERT(ei); OSMO_ASSERT(ei->key_len <= ARRAY_SIZE(ei->key)); OSMO_ASSERT(ei->perm_algo_len <= ENCRY_INFO_PERM_ALGO_MAXLEN); @@ -751,7 +734,6 @@ unsigned int perm_algo_len = 0; const uint8_t *old_elem = elem; - OSMO_ASSERT(ei); if (!elem) return -EINVAL; if (len == 0) @@ -1011,9 +993,6 @@ unsigned int i; uint8_t id_discr; - OSMO_ASSERT(msg); - OSMO_ASSERT(cil); - msgb_put_u8(msg, GSM0808_IE_CELL_IDENTIFIER_LIST); tlv_len = msgb_put(msg, 1); old_tail = msg->tail; @@ -1055,9 +1034,6 @@ uint8_t *tlv_len; unsigned int i; - OSMO_ASSERT(msg); - OSMO_ASSERT(cil); - msgb_put_u8(msg, GSM0808_IE_CELL_IDENTIFIER_LIST); tlv_len = msgb_put(msg, 1); old_tail = msg->tail; @@ -1233,7 +1209,6 @@ size_t bytes_elem = 0; int list_len = 0; - OSMO_ASSERT(cil); if (!elem) return -EINVAL; if (len == 0) @@ -1301,7 +1276,6 @@ const uint8_t *old_elem = elem; unsigned int item_count = 0; - OSMO_ASSERT(cil); if (!elem) return -EINVAL; if (len == 0) @@ -1459,9 +1433,6 @@ .id_list_len = 1, }; - OSMO_ASSERT(msg); - OSMO_ASSERT(ci); - ie_tag = msg->tail; rc = gsm0808_enc_cell_id_list2(msg, &cil); -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/30579 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: If9b4c92ace68191f5ddcc0a8a340fccbfe0f3dc0 Gerrit-Change-Number: 30579 Gerrit-PatchSet: 1 Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de> Gerrit-MessageType: merged

2 years, 6 months

1
0
0 0

Change in osmo-pcu[master]: Get rid of tbf->first_ts

by fixeria

Attention is currently required from: laforge, pespin. fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/30568 ) Change subject: Get rid of tbf->first_ts ...................................................................... Patch Set 1: (3 comments) Commit Message: https://gerrit.osmocom.org/c/osmo-pcu/+/30568/comment/a68d28c1_ec6c932c PS1, Line 9: quickly Not going to block you work, but are you sure this is not causing any noticeable performance regressions? Caching first_ts might have been done on purpose, e.g. to avoid iterating over the list of PDCHs in hot code paths. I am not familiar with the code base as good as you are, just making sure we're still good after this change. File src/tbf.cpp: https://gerrit.osmocom.org/c/osmo-pcu/+/30568/comment/cf309207_34addf75 PS1, Line 899: 8 > I don't really like the magic numbers. […] Ack, ARRAY_SIZE(tbf->pdch) should work here. https://gerrit.osmocom.org/c/osmo-pcu/+/30568/comment/f1a8654a_434daac8 PS1, Line 911: 8 Same here. -- To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/30568 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Change-Id: I5d2f665f648f8637466bfdd3bf7b924cb61ede33 Gerrit-Change-Number: 30568 Gerrit-PatchSet: 1 Gerrit-Owner: pespin <pespin(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-CC: laforge <laforge(a)osmocom.org> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Tue, 13 Dec 2022 22:28:08 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: laforge <laforge(a)osmocom.org> Gerrit-MessageType: comment

2 years, 6 months

1
0
0 0

Change in libosmocore[master]: gsm0808: add logging for some IE encoding errors

by neels

Attention is currently required from: fixeria. neels has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/30581 ) Change subject: gsm0808: add logging for some IE encoding errors ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: it's nontrivial to add the first logging to a library (part of a library): Before this patch, gsm0808.c can be used without osmo_log_init(). Starting with this patch, all users must initialize logging. in the context of osmocom, we know that all programs use osmo logging. but do we lose flexibility? category DLGLOBAL is a bad fit, should we add DLBSSAP now? a solution would be an error_cb(), concise error info is provided by the lib, the caller decides how to handle it. should we also add logging and better handling for all other error cases in this file? i'd just leave it as it is... -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/30581 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Idd9c490e7a2d37817004590629092c4bb6f2d758 Gerrit-Change-Number: 30581 Gerrit-PatchSet: 1 Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: neels <nhofmeyr(a)sysmocom.de> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Comment-Date: Tue, 13 Dec 2022 22:18:22 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

2 years, 6 months

1
0
0 0

Change in libosmocore[master]: gsm0808: use new gsm0808_enc_speech_codec[_list]2() API

by neels

Attention is currently required from: fixeria. neels has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/30580 ) Change subject: gsm0808: use new gsm0808_enc_speech_codec[_list]2() API ...................................................................... Patch Set 1: Code-Review+2 (1 comment) Commit Message: https://gerrit.osmocom.org/c/libosmocore/+/30580/comment/01a7637e_c1fc0ce5 PS1, Line 10: preceeding preceding not to be confused with preseeding =) -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/30580 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: I28219b61b9347f0652f9fd0c717f6cdf3c63e8f9 Gerrit-Change-Number: 30580 Gerrit-PatchSet: 1 Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Comment-Date: Tue, 13 Dec 2022 22:08:44 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

2 years, 6 months

1
0
0 0

Change in osmo-pcu[master]: Move first_common_ts from gprs_rlcmac_tbf to GprsMs

by fixeria

Attention is currently required from: laforge, pespin. fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/30566 ) Change subject: Move first_common_ts from gprs_rlcmac_tbf to GprsMs ...................................................................... Patch Set 2: (2 comments) File src/gprs_ms.h: https://gerrit.osmocom.org/c/osmo-pcu/+/30566/comment/a8c4d44e_48d8c5b3 PS2, Line 67: simultaniously > I think "simultaneously" is somewhat misleading here, given that the UL and DL TS have an offset and […] AFAICS, this comment is simply copied as-is from src/tbf.h#b223, except the value range. File src/pcu_vty_functions.cpp: https://gerrit.osmocom.org/c/osmo-pcu/+/30566/comment/44c459a2_dc14e822 PS2, Line 64: PRId8 Is there really a need for this change? %d would still work for int8_t, AFAIK. -- To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/30566 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Change-Id: I8df01a99ccbfaf7a442ade5000ee282bd638fbba Gerrit-Change-Number: 30566 Gerrit-PatchSet: 2 Gerrit-Owner: pespin <pespin(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-CC: laforge <laforge(a)osmocom.org> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Tue, 13 Dec 2022 22:07:21 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: laforge <laforge(a)osmocom.org> Gerrit-MessageType: comment

2 years, 6 months

1
0
0 0

Change in libosmocore[master]: gsm0808: add gsm0808_enc_speech_codec[_list]2()

by neels

Attention is currently required from: fixeria. neels has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/30574 ) Change subject: gsm0808: add gsm0808_enc_speech_codec[_list]2() ...................................................................... Patch Set 2: Code-Review+2 (2 comments) File src/gsm/gsm0808_utils.c: https://gerrit.osmocom.org/c/libosmocore/+/30574/comment/83ad3ec4_657c8981 PS1, Line 315: return rc; > I thought about this too, but IMO trying to recover the given msgb would unnecessary complicate thin […] Done https://gerrit.osmocom.org/c/libosmocore/+/30574/comment/e2269320_73f1b9a3 PS1, Line 434: old_tail = msg->tail; > same as above, "restore" msgb on error? Done -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/30574 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: I199ffa0ba4a64813238519178155dfc767aa3975 Gerrit-Change-Number: 30574 Gerrit-PatchSet: 2 Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Comment-Date: Tue, 13 Dec 2022 22:04:11 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: neels <nhofmeyr(a)sysmocom.de> Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-MessageType: comment

2 years, 6 months

1
0
0 0

Change in libosmocore[master]: gsm0808: remove over-defensive assert()s for function parameters

by neels

Attention is currently required from: fixeria. neels has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/30579 ) Change subject: gsm0808: remove over-defensive assert()s for function parameters ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/30579 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: If9b4c92ace68191f5ddcc0a8a340fccbfe0f3dc0 Gerrit-Change-Number: 30579 Gerrit-PatchSet: 1 Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Comment-Date: Tue, 13 Dec 2022 22:00:54 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

2 years, 6 months

1
0
0 0

2025

2024

2023

2022

gerrit-log December 2022