laforge has submitted this change. ( https://gerrit.osmocom.org/c/libosmocore/+/31505 ) Change subject: gsm/{bsslap,bssmap_le}: zero-initialize structs using memset() ...................................................................... gsm/{bsslap,bssmap_le}: zero-initialize structs using memset() In the unit tests we're using memcmp() to compare decoding results against the expected results. This is a reasonable approach, but there is a pitfall: not only the struct fields are compared, but also the padding bytes preceding/following them. When using gcc's extension zero-initializer {} or even the standard approved { 0 } zero-initializer, padding bytes are not guaranteed to be zeroed. Even worse, according to [1], the init behavior is inconsistent between gcc and clang and optimization levels. All decoding functions in {bsslap,bssmap_le}.c currently use gcc's extension zero-initializer {}. This is not a problem when building with CC=gcc, but with CC=clang the bssmap_le_test fails due to mismatch of padding bytes in struct lcs_cause_ie: [4] PERFORM LOCATION RESPONSE: ERROR: decoded PDU != encoded PDU [5] PERFORM LOCATION RESPONSE: ERROR: decoded PDU != encoded PDU [6] PERFORM LOCATION ABORT: ERROR: decoded PDU != encoded PDU Out of the known struct initialization methods, only the memset() has consistent behavior and sets all bytes to zero, including the padding ones. Using it fixes the bssmap_le_test for CC=clang. [1] https://interrupt.memfault.com/blog/c-struct-padding-initialization Change-Id: Ib16964b16eb04315efc416164ed46c15b5dc7254 Fixes: OS#5923 --- M src/gsm/bsslap.c M src/gsm/bssmap_le.c 2 files changed, 45 insertions(+), 9 deletions(-) Approvals: laforge: Looks good to me, approved neels: Looks good to me, but someone else must approve Jenkins Builder: Verified diff --git a/src/gsm/bsslap.c b/src/gsm/bsslap.c index 7b31d1f..70ded13 100644 --- a/src/gsm/bsslap.c +++ b/src/gsm/bsslap.c @@ -217,7 +217,7 @@ int ies_len; struct tlv_parsed tp; - *pdu = (struct bsslap_pdu){}; + memset(pdu, 0x00, sizeof(*pdu)); if (err) *err = NULL; diff --git a/src/gsm/bssmap_le.c b/src/gsm/bssmap_le.c index 3e90a43..1ee4551 100644 --- a/src/gsm/bssmap_le.c +++ b/src/gsm/bssmap_le.c @@ -180,7 +180,7 @@ struct osmo_bssmap_le_err **err, void *err_ctx, const uint8_t *elem, uint8_t len) { - *lt = (struct bssmap_le_location_type){}; + memset(lt, 0x00, sizeof(*lt)); if (!elem || len < 1) DEC_ERR(-EINVAL, msgt, iei, LCS_CAUSE_UNSPECIFIED, "zero length"); @@ -348,7 +348,7 @@ struct osmo_bssmap_le_err **err, void *err_ctx, const uint8_t *data, uint8_t len) { - *lcs_cause = (struct lcs_cause_ie){}; + memset(lcs_cause, 0x00, sizeof(*lcs_cause)); if (!data || len < 1) DEC_ERR(-EINVAL, msgt, iei, LCS_CAUSE_UNSPECIFIED, "zero length"); @@ -558,7 +558,7 @@ struct osmo_bssmap_le_err **err, void *err_ctx, const struct tlv_parsed *tp) { - *params = (struct bssmap_le_perform_loc_req){}; + memset(params, 0x00, sizeof(*params)); DEC_IE_MANDATORY(msgt, BSSMAP_LE_IEI_LOCATION_TYPE, osmo_bssmap_le_ie_dec_location_type, &params->location_type); @@ -606,7 +606,7 @@ struct osmo_bssmap_le_err **err, void *err_ctx, const struct tlv_parsed *tp) { - *params = (struct bssmap_le_perform_loc_resp){}; + memset(params, 0x00, sizeof(*params)); DEC_IE_OPTIONAL_FLAG(msgt, BSSMAP_LE_IEI_GEO_LOCATION, osmo_bssmap_le_ie_dec_gad, &params->location_estimate, params->location_estimate_present); @@ -630,7 +630,7 @@ struct osmo_bssmap_le_err **err, void *err_ctx, const struct tlv_parsed *tp) { - *params = (struct lcs_cause_ie){}; + memset(params, 0x00, sizeof(*params)); DEC_IE_MANDATORY(msgt, BSSMAP_LE_IEI_LCS_CAUSE, osmo_lcs_cause_dec, params); return 0; @@ -647,7 +647,8 @@ struct osmo_bssmap_le_err **err, void *err_ctx, const struct tlv_parsed *tp) { - *params = (struct bssmap_le_conn_oriented_info){}; + memset(params, 0x00, sizeof(*params)); + DEC_IE_MANDATORY(msgt, BSSMAP_LE_IEI_APDU, osmo_bssmap_le_ie_dec_apdu, &params->apdu); return 0; } @@ -711,7 +712,7 @@ int ies_len; struct tlv_parsed tp; - *pdu = (struct bssmap_le_pdu){}; + memset(pdu, 0x00, sizeof(*pdu)); if (len < 1) DEC_ERR(-EINVAL, -1, -1, LCS_CAUSE_UNSPECIFIED, "zero length"); @@ -798,7 +799,7 @@ return RC; \ } while(0) - *pdu = (struct bssap_le_pdu){}; + memset(pdu, 0x00, sizeof(*pdu)); h = msgb_l2(msg); if (!h) -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/31505 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Ib16964b16eb04315efc416164ed46c15b5dc7254 Gerrit-Change-Number: 31505 Gerrit-PatchSet: 2 Gerrit-Owner: fixeria &lt;vyanitskiy(a)sysmocom.de&gt; Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge &lt;laforge(a)osmocom.org&gt; Gerrit-Reviewer: neels &lt;nhofmeyr(a)sysmocom.de&gt; Gerrit-MessageType: merged