Attention is currently required from: pespin.

View Change

1 comment:

• File src/osmo-bsc-nat/bssap_conn.c:

  • Patch Set #2, Line 60: len = IP_V4_ADDR_LEN;

    Thanks for the suggestion!

    gsm0808_enc_aoip_trasp_addr should take care of checking internally whether there's space in msg_new.

    I read related libosmocore code, and now understand that all msgb code works like this, using msgb_put(msg, len) to get a pointer to a buffer to write len bytes to, and then actually write it with e.g. memcpy afterwards. msgb_put already checks that enough space is available: https://gitea.osmocom.org/osmocom/libosmocore/src/commit/0578c288ec9e33512cfcfa0f83f669ca98a469e6/include/osmocom/core/msgb.h#L237

    So that's how the check for gsm0808_enc_aoip_trasp_addr is performed. tlv_encode_one is also using msgb functions so the length check should also work there. I think CID#273004 is a false positive now, it seems coverity can't follow that msgb_tvlv_put() would already check the length through msgb_put(msg, TVLV_GROSS_LEN(len)) correctly before writing to it.

    Do you agree that it's a false positive?

    Regarding whether to check length here: I think we should either check the length for both cases (copy IE over and replacing it) like it's done in the current version of the patch, or not check the length at all and rely on msgb_put to do it. By adding the check here we could avoid osmo_panic() if there's not enough space, but it makes the code here more complicated.

    Probably not worth it?

To view, visit change 28582. To unsubscribe, or for help writing mail filters, visit settings.