Neels Hofmeyr has submitted this change and it was merged. ( https://gerrit.osmocom.org/10754 )
Change subject: fix mgcp_verify_ci(): off-by-one in max len check ...................................................................... fix mgcp_verify_ci(): off-by-one in max len check MGCP_CONN_ID_MAXLEN actually includes a terminating nul, so we need to compare strlen() against MGCP_CONN_ID_MAXLEN-1. Log the length if it is too long. Add MDCX_TOO_LONG_CI test to mgcp_test.c, testing a conn id of 33 characters. Before this patch, the test returns error code 515 meaning "not found", while now it returns 510 meaning "invalid", showing the off-by-one. Same is illustrated by the error log ("not found" before, "too long" now), but the error log is not verified by mgcp_test.c. Change-Id: I8d6cc96be252bb486e94f343a8c7cae641ff9429 --- M src/libosmo-mgcp/mgcp_msg.c M tests/mgcp/mgcp_test.c M tests/mgcp/mgcp_test.ok 3 files changed, 23 insertions(+), 3 deletions(-) Approvals: Jenkins Builder: Verified Harald Welte: Looks good to me, approved diff --git a/src/libosmo-mgcp/mgcp_msg.c b/src/libosmo-mgcp/mgcp_msg.c index f732158..648d86b 100644 --- a/src/libosmo-mgcp/mgcp_msg.c +++ b/src/libosmo-mgcp/mgcp_msg.c @@ -454,10 +454,10 @@ } /* Check for over long connection identifiers */ - if (strlen(conn_id) > MGCP_CONN_ID_MAXLEN) { + if (strlen(conn_id) > (MGCP_CONN_ID_MAXLEN-1)) { LOGP(DLMGCP, LOGL_ERROR, - "endpoint:0x%x invalid ConnectionIdentifier (too long) 0x%s\n", - ENDPOINT_NUMBER(endp), conn_id); + "endpoint:0x%x invalid ConnectionIdentifier (too long: %zu > %d) 0x%s\n", + ENDPOINT_NUMBER(endp), strlen(conn_id), MGCP_CONN_ID_MAXLEN-1, conn_id); return 510; } diff --git a/tests/mgcp/mgcp_test.c b/tests/mgcp/mgcp_test.c index 2f3a8a7..c40eabc 100644 --- a/tests/mgcp/mgcp_test.c +++ b/tests/mgcp/mgcp_test.c @@ -230,6 +230,12 @@ "I: %s\r\n" \ "L: p:20, a:AMR, nt:IN\r\n" +#define MDCX_TOO_LONG_CI \ + "MDCX 18983222 1@mgw MGCP 1.0\r\n" \ + "I: 123456789012345678901234567890123\n" + +#define MDCX_TOO_LONG_CI_RET "510 18983222 FAIL\r\n" + #define SHORT2 "CRCX 1" #define SHORT2_RET "510 000000 FAIL\r\n" #define SHORT3 "CRCX 1 1@mgw" @@ -510,6 +516,7 @@ {"DLCX", DLCX, DLCX_RET, PTYPE_IGNORE,.extra_fmtp = "a=fmtp:126 0/1/2"}, {"CRCX", CRCX_NO_LCO_NO_SDP, CRCX_NO_LCO_NO_SDP_RET, 97}, {"CRCX", CRCX_X_OSMO_IGN, CRCX_X_OSMO_IGN_RET, 97}, + {"MDCX_TOO_LONG_CI", MDCX_TOO_LONG_CI, MDCX_TOO_LONG_CI_RET}, }; static const struct mgcp_test retransmit[] = { diff --git a/tests/mgcp/mgcp_test.ok b/tests/mgcp/mgcp_test.ok index 915d45e..ddda751 100644 --- a/tests/mgcp/mgcp_test.ok +++ b/tests/mgcp/mgcp_test.ok @@ -443,6 +443,19 @@ Dummy packets: 2 ================================================ +Testing MDCX_TOO_LONG_CI +creating message from statically defined input: +---------8<--------- +MDCX 18983222 1@mgw MGCP 1.0 +I: 123456789012345678901234567890123 + +---------8<--------- +checking response: +using message as statically defined for comparison +Response matches our expectations. +(response does not contain a connection id) + +================================================ Testing CRCX creating message from statically defined input: ---------8<--------- -- To view, visit https://gerrit.osmocom.org/10754 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-mgw Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I8d6cc96be252bb486e94f343a8c7cae641ff9429 Gerrit-Change-Number: 10754 Gerrit-PatchSet: 1 Gerrit-Owner: Neels Hofmeyr <nhofm...@sysmocom.de> Gerrit-Reviewer: Harald Welte <lafo...@gnumonks.org> Gerrit-Reviewer: Jenkins Builder (1000002) Gerrit-Reviewer: Neels Hofmeyr <nhofm...@sysmocom.de>