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/.
Pau Espin Pedrol gerrit-no-reply at lists.osmocom.orgPau Espin Pedrol has uploaded this change for review. ( https://gerrit.osmocom.org/11381 Change subject: osmux: Improve checks around activating and using enabled osmux ...................................................................... osmux: Improve checks around activating and using enabled osmux * Refactor code to have unified checks on all paths activating Osmux. * Improve checkings at activation time and add logging. * Code now enforces endp osmux status to be enabled before processing the frame through endp->osmux.out. Before, a delayed or bad pkt could arrive and be processed by an endp with osmux not enabled, using endp->osmux.out that was not initialized and ended up crashing: libosmo-netif/src/osmux.c:281:3: runtime error: member access within null pointer of type 'struct msgb' This could also happen if a BSC started sending or we received (non legacy dummy) osmux frames before we received the BSC CRCX ACK agreeing on osmux negotiation and switching to ACTIVATING state. Related: SYS#4350 Port from openbsc 4a2cc9eb0a0f9424c16b26fcb757483a39d67482. Includes fixup from openbsc I438349bffaa46a10ad8983090a4b17aed7e00d82. Change-Id: Iac11e447ec0d76e4e74ec982a6e3f63b35548978 --- M src/libosmo-mgcp/mgcp_osmux.c 1 file changed, 43 insertions(+), 41 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-mgw refs/changes/81/11381/1 diff --git a/src/libosmo-mgcp/mgcp_osmux.c b/src/libosmo-mgcp/mgcp_osmux.c index 2828d83..ca446ff 100644 --- a/src/libosmo-mgcp/mgcp_osmux.c +++ b/src/libosmo-mgcp/mgcp_osmux.c @@ -316,6 +316,34 @@ return msg; } +/* Updates endp osmux state and returns 0 if it can process messages, -1 otherwise */ +static int endp_osmux_state_check(struct mgcp_endpoint *endp, struct mgcp_conn_rtp *conn, + bool sending) +{ + switch(conn->osmux.state) { + case OSMUX_STATE_ACTIVATING: + if (osmux_enable_conn(endp, conn, &conn->end.addr, htons(endp->cfg->osmux_port)) < 0) { + LOGP(DLMGCP, LOGL_ERROR, + "Could not enable osmux for conn:%s\n", + mgcp_conn_dump(conn->conn)); + return -1; + } + LOGP(DLMGCP, LOGL_ERROR, + "Osmux CID %u for %s:%u is now enabled\n", + conn->osmux.cid, inet_ntoa(conn->end.addr), + endp->cfg->osmux_port); + return 0; + case OSMUX_STATE_ENABLED: + return 0; + default: + LOGP(DLMGCP, LOGL_ERROR, + "Osmux %s in conn %s without full negotiation, state %d\n", + sending ? "sent" : "received", + mgcp_conn_dump(conn->conn), conn->osmux.state); + return -1; + } +} + static int osmux_legacy_dummy_parse_cid(struct sockaddr_in *addr, struct msgb *msg, uint8_t *osmux_cid) { @@ -374,11 +402,12 @@ osmuxh->circuit_id); goto out; } - conn_bts->osmux.stats.octets += osmux_chunk_length(msg, rem); - conn_bts->osmux.stats.chunks++; + if (endp_osmux_state_check(endp, conn_bts, false) == 0) { + conn_bts->osmux.stats.octets += osmux_chunk_length(msg, rem); + conn_bts->osmux.stats.chunks++; + osmux_xfrm_output_sched(&conn_bts->osmux.out, osmuxh); + } rem = msg->len; - - osmux_xfrm_output_sched(&conn_bts->osmux.out, osmuxh); } out: msgb_free(msg); @@ -408,19 +437,8 @@ if (!conn) goto out; - if (conn->osmux.state == OSMUX_STATE_ENABLED) - goto out; - - if (osmux_enable_conn(endp, conn, &addr->sin_addr, addr->sin_port) < 0 ) { - LOGP(DLMGCP, LOGL_ERROR, - "Could not enable osmux in endpoint 0x%x\n", - ENDPOINT_NUMBER(endp)); - goto out; - } - - LOGP(DLMGCP, LOGL_INFO, "Enabling osmux in endpoint 0x%x for %s:%u\n", - ENDPOINT_NUMBER(endp), inet_ntoa(addr->sin_addr), - ntohs(addr->sin_port)); + endp_osmux_state_check(endp, conn, false); + /* Only needed to punch hole in firewall, it can be dropped */ out: msgb_free(msg); return 0; @@ -468,11 +486,12 @@ osmuxh->circuit_id); goto out; } - conn_net->osmux.stats.octets += osmux_chunk_length(msg, rem); - conn_net->osmux.stats.chunks++; + if (endp_osmux_state_check(endp, conn_net, false) == 0) { + conn_net->osmux.stats.octets += osmux_chunk_length(msg, rem); + conn_net->osmux.stats.chunks++; + osmux_xfrm_output_sched(&conn_net->osmux.out, osmuxh); + } rem = msg->len; - - osmux_xfrm_output_sched(&conn_net->osmux.out, osmuxh); } out: msgb_free(msg); @@ -647,26 +666,9 @@ if (memcmp(&conn->end.addr, &addr_unset, sizeof(addr_unset)) == 0) return 0; - if (conn->osmux.state == OSMUX_STATE_ACTIVATING) { - if (osmux_enable_conn(endp, conn, &conn->end.addr, - htons(endp->cfg->osmux_port)) < 0) { - LOGP(DLMGCP, LOGL_ERROR, - "Could not activate osmux for conn:%s\n", - mgcp_conn_dump(conn->conn)); - return 0; - } - LOGP(DLMGCP, LOGL_ERROR, - "Osmux CID %u for %s:%u is now enabled\n", - conn->osmux.cid, inet_ntoa(conn->end.addr), - endp->cfg->osmux_port); - } - if(conn->osmux.state != OSMUX_STATE_ENABLED) { - LOGP(DLMGCP, LOGL_ERROR, - "OSMUX dummy to %s CID %u: Osmux not enabled on endpoint 0x%x state %d\n", - inet_ntoa(conn->end.addr), conn->osmux.cid, - ENDPOINT_NUMBER(endp), conn->osmux.state); - return 0; - } + if (endp_osmux_state_check(endp, conn, true) < 0) + return 0; + LOGP(DLMGCP, LOGL_DEBUG, "sending OSMUX dummy load to %s CID %u\n", inet_ntoa(conn->end.addr), conn->osmux.cid); -- To view, visit https://gerrit.osmocom.org/11381 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-mgw Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Iac11e447ec0d76e4e74ec982a6e3f63b35548978 Gerrit-Change-Number: 11381 Gerrit-PatchSet: 1 Gerrit-Owner: Pau Espin Pedrol <pespin at sysmocom.de> -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20181016/c7dfda9d/attachment.htm>