mgcp_client: add transaction cleanup
So far, if an MGCP message is sent, the transaction gets enqueued, but there is no way to end the transaction other than receiving a valid reply. So, if the caller decides that the transaction timed out and tears down the priv pointer passed to mgcp_client_tx, and if then a late reply arrives, the callback will dereference the invalid priv pointer and cause a segfault. Hence it is possible to crash an mgcp_client program by sending a late response. Furthermore, if no reply ever arrives, we would keep the pending response in the list forever, amounting to a "memory leak". Add mgcp_client_cancel() to discard a pending transaction. The caller can now decide to discard a pending response when it sees fit (e.g. the caller's timeout expired). This needs to be added to OsmoMSC and OsmoBSC. Add mgcp_msg_trans_id() to provide an obvious way to obtain the transaction id from a generated MGCP message. No public API is broken; but refine the negative return code from mgcp_client_rx(): return -ENOENT if no such transaction ID is found, and still -1 if decoding failed. This is mainly for mgcp_client_test. Implement a test for mgcp_client_cancel() in mgcp_client_test.c. Tweak internal mgcp_client_response_pending_get() to take only the transaction id as argument instead of the entire mgcp message struct. Found-by: dexter Related: OS#2695 OS#2696 Change-Id: I16811e168a46a82a05943252a737b3434143f4bd
This commit is contained in:
parent
c0dcc3c60c
commit
c8f37cb4d6
|
@ -93,6 +93,7 @@ int mgcp_response_parse_params(struct mgcp_response *r);
|
||||||
|
|
||||||
int mgcp_client_tx(struct mgcp_client *mgcp, struct msgb *msg,
|
int mgcp_client_tx(struct mgcp_client *mgcp, struct msgb *msg,
|
||||||
mgcp_response_cb_t response_cb, void *priv);
|
mgcp_response_cb_t response_cb, void *priv);
|
||||||
|
int mgcp_client_cancel(struct mgcp_client *mgcp, mgcp_trans_id_t trans_id);
|
||||||
|
|
||||||
enum mgcp_connection_mode;
|
enum mgcp_connection_mode;
|
||||||
|
|
||||||
|
@ -111,6 +112,7 @@ struct msgb *mgcp_msg_dlcx(struct mgcp_client *mgcp, uint16_t rtp_endpoint,
|
||||||
OSMO_DEPRECATED("Use mgcp_msg_gen() instead");
|
OSMO_DEPRECATED("Use mgcp_msg_gen() instead");
|
||||||
|
|
||||||
struct msgb *mgcp_msg_gen(struct mgcp_client *mgcp, struct mgcp_msg *mgcp_msg);
|
struct msgb *mgcp_msg_gen(struct mgcp_client *mgcp, struct mgcp_msg *mgcp_msg);
|
||||||
|
mgcp_trans_id_t mgcp_msg_trans_id(struct msgb *msg);
|
||||||
|
|
||||||
extern const struct value_string mgcp_client_connection_mode_strs[];
|
extern const struct value_string mgcp_client_connection_mode_strs[];
|
||||||
static inline const char *mgcp_client_cmode_name(enum mgcp_connection_mode mode)
|
static inline const char *mgcp_client_cmode_name(enum mgcp_connection_mode mode)
|
||||||
|
|
|
@ -311,13 +311,11 @@ exit:
|
||||||
|
|
||||||
static struct mgcp_response_pending *mgcp_client_response_pending_get(
|
static struct mgcp_response_pending *mgcp_client_response_pending_get(
|
||||||
struct mgcp_client *mgcp,
|
struct mgcp_client *mgcp,
|
||||||
struct mgcp_response *r)
|
mgcp_trans_id_t trans_id)
|
||||||
{
|
{
|
||||||
struct mgcp_response_pending *pending;
|
struct mgcp_response_pending *pending;
|
||||||
if (!r)
|
|
||||||
return NULL;
|
|
||||||
llist_for_each_entry(pending, &mgcp->responses_pending, entry) {
|
llist_for_each_entry(pending, &mgcp->responses_pending, entry) {
|
||||||
if (pending->trans_id == r->head.trans_id) {
|
if (pending->trans_id == trans_id) {
|
||||||
llist_del(&pending->entry);
|
llist_del(&pending->entry);
|
||||||
return pending;
|
return pending;
|
||||||
}
|
}
|
||||||
|
@ -349,12 +347,12 @@ int mgcp_client_rx(struct mgcp_client *mgcp, struct msgb *msg)
|
||||||
return -1;
|
return -1;
|
||||||
}
|
}
|
||||||
|
|
||||||
pending = mgcp_client_response_pending_get(mgcp, &r);
|
pending = mgcp_client_response_pending_get(mgcp, r.head.trans_id);
|
||||||
if (!pending) {
|
if (!pending) {
|
||||||
LOGP(DLMGCP, LOGL_ERROR,
|
LOGP(DLMGCP, LOGL_ERROR,
|
||||||
"Cannot find matching MGCP transaction for trans_id %d\n",
|
"Cannot find matching MGCP transaction for trans_id %d\n",
|
||||||
r.head.trans_id);
|
r.head.trans_id);
|
||||||
return -1;
|
return -ENOENT;
|
||||||
}
|
}
|
||||||
|
|
||||||
mgcp_client_handle_response(mgcp, pending, &r);
|
mgcp_client_handle_response(mgcp, pending, &r);
|
||||||
|
@ -560,7 +558,10 @@ struct mgcp_response_pending * mgcp_client_pending_add(
|
||||||
* response_cb. NOTE: the response_cb still needs to call
|
* response_cb. NOTE: the response_cb still needs to call
|
||||||
* mgcp_response_parse_params(response) to get the parsed parameters -- to
|
* mgcp_response_parse_params(response) to get the parsed parameters -- to
|
||||||
* potentially save some CPU cycles, only the head line has been parsed when
|
* potentially save some CPU cycles, only the head line has been parsed when
|
||||||
* the response_cb is invoked. */
|
* the response_cb is invoked.
|
||||||
|
* Before the priv pointer becomes invalid, e.g. due to transaction timeout,
|
||||||
|
* mgcp_client_cancel() needs to be called for this transaction.
|
||||||
|
*/
|
||||||
int mgcp_client_tx(struct mgcp_client *mgcp, struct msgb *msg,
|
int mgcp_client_tx(struct mgcp_client *mgcp, struct msgb *msg,
|
||||||
mgcp_response_cb_t response_cb, void *priv)
|
mgcp_response_cb_t response_cb, void *priv)
|
||||||
{
|
{
|
||||||
|
@ -603,6 +604,32 @@ mgcp_tx_error:
|
||||||
return -1;
|
return -1;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/* Cancel a pending transaction.
|
||||||
|
* Should a priv pointer passed to mgcp_client_tx() become invalid, this function must be called. In
|
||||||
|
* practical terms, if the caller of mgcp_client_tx() wishes to tear down a transaction without having
|
||||||
|
* received a response this function must be called. The trans_id can be obtained by calling
|
||||||
|
* mgcp_msg_trans_id() on the msgb produced by mgcp_msg_gen().
|
||||||
|
*/
|
||||||
|
int mgcp_client_cancel(struct mgcp_client *mgcp, mgcp_trans_id_t trans_id)
|
||||||
|
{
|
||||||
|
struct mgcp_response_pending *pending = mgcp_client_response_pending_get(mgcp, trans_id);
|
||||||
|
if (!pending) {
|
||||||
|
/* INFO is sufficient, it is not harmful to cancel a transaction twice. */
|
||||||
|
LOGP(DLMGCP, LOGL_INFO, "Cannot cancel, no such transaction: %u\n", trans_id);
|
||||||
|
return -ENOENT;
|
||||||
|
}
|
||||||
|
LOGP(DLMGCP, LOGL_INFO, "Canceled transaction %u\n", trans_id);
|
||||||
|
talloc_free(pending);
|
||||||
|
return 0;
|
||||||
|
/* We don't really need to clean up the wqueue: In all sane cases, the msgb has already been sent
|
||||||
|
* out and is no longer in the wqueue. If it still is in the wqueue, then sending MGCP messages
|
||||||
|
* per se is broken and the program should notice so by a full wqueue. Even if this was called
|
||||||
|
* before we had a chance to send out the message and it is still going to be sent, we will just
|
||||||
|
* ignore the reply to it later. Removing a msgb from the wqueue here would just introduce more
|
||||||
|
* bug surface in terms of failing to update wqueue API's counters or some such.
|
||||||
|
*/
|
||||||
|
}
|
||||||
|
|
||||||
static struct msgb *mgcp_msg_from_buf(mgcp_trans_id_t trans_id,
|
static struct msgb *mgcp_msg_from_buf(mgcp_trans_id_t trans_id,
|
||||||
const char *buf, int len)
|
const char *buf, int len)
|
||||||
{
|
{
|
||||||
|
@ -806,6 +833,12 @@ struct msgb *mgcp_msg_gen(struct mgcp_client *mgcp, struct mgcp_msg *mgcp_msg)
|
||||||
return msg;
|
return msg;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/* Retrieve the MGCP transaction ID from a msgb generated by mgcp_msg_gen() */
|
||||||
|
mgcp_trans_id_t mgcp_msg_trans_id(struct msgb *msg)
|
||||||
|
{
|
||||||
|
return (mgcp_trans_id_t)msg->cb[MSGB_CB_MGCP_TRANS_ID];
|
||||||
|
}
|
||||||
|
|
||||||
struct mgcp_client_conf *mgcp_client_conf_actual(struct mgcp_client *mgcp)
|
struct mgcp_client_conf *mgcp_client_conf_actual(struct mgcp_client *mgcp)
|
||||||
{
|
{
|
||||||
return &mgcp->actual;
|
return &mgcp->actual;
|
||||||
|
|
|
@ -24,6 +24,7 @@
|
||||||
#include <osmocom/core/application.h>
|
#include <osmocom/core/application.h>
|
||||||
#include <osmocom/mgcp_client/mgcp_client.h>
|
#include <osmocom/mgcp_client/mgcp_client.h>
|
||||||
#include <osmocom/mgcp_client/mgcp_client_internal.h>
|
#include <osmocom/mgcp_client/mgcp_client_internal.h>
|
||||||
|
#include <errno.h>
|
||||||
|
|
||||||
void *ctx;
|
void *ctx;
|
||||||
|
|
||||||
|
@ -73,7 +74,7 @@ static struct msgb *from_str(const char *str)
|
||||||
static struct mgcp_client_conf conf;
|
static struct mgcp_client_conf conf;
|
||||||
struct mgcp_client *mgcp = NULL;
|
struct mgcp_client *mgcp = NULL;
|
||||||
|
|
||||||
static void reply_to(mgcp_trans_id_t trans_id, int code, const char *comment,
|
static int reply_to(mgcp_trans_id_t trans_id, int code, const char *comment,
|
||||||
int conn_id, const char *params)
|
int conn_id, const char *params)
|
||||||
{
|
{
|
||||||
static char compose[4096 - 128];
|
static char compose[4096 - 128];
|
||||||
|
@ -87,7 +88,7 @@ static void reply_to(mgcp_trans_id_t trans_id, int code, const char *comment,
|
||||||
|
|
||||||
printf("composed response:\n-----\n%s\n-----\n",
|
printf("composed response:\n-----\n%s\n-----\n",
|
||||||
compose);
|
compose);
|
||||||
mgcp_client_rx(mgcp, from_str(compose));
|
return mgcp_client_rx(mgcp, from_str(compose));
|
||||||
}
|
}
|
||||||
|
|
||||||
void test_response_cb(struct mgcp_response *response, void *priv)
|
void test_response_cb(struct mgcp_response *response, void *priv)
|
||||||
|
@ -225,6 +226,51 @@ void test_mgcp_msg(void)
|
||||||
msgb_free(msg);
|
msgb_free(msg);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
void test_mgcp_client_cancel()
|
||||||
|
{
|
||||||
|
mgcp_trans_id_t trans_id;
|
||||||
|
struct msgb *msg;
|
||||||
|
struct mgcp_msg mgcp_msg = {
|
||||||
|
.verb = MGCP_VERB_CRCX,
|
||||||
|
.audio_ip = "192.168.100.23",
|
||||||
|
.endpoint = "23@mgw",
|
||||||
|
.audio_port = 1234,
|
||||||
|
.call_id = 47,
|
||||||
|
.conn_id = 11,
|
||||||
|
.conn_mode = MGCP_CONN_RECV_SEND,
|
||||||
|
.presence = (MGCP_MSG_PRESENCE_ENDPOINT | MGCP_MSG_PRESENCE_CALL_ID
|
||||||
|
| MGCP_MSG_PRESENCE_CONN_ID | MGCP_MSG_PRESENCE_CONN_MODE),
|
||||||
|
};
|
||||||
|
|
||||||
|
printf("\n%s():\n", __func__);
|
||||||
|
fprintf(stderr, "\n%s():\n", __func__);
|
||||||
|
|
||||||
|
if (mgcp)
|
||||||
|
talloc_free(mgcp);
|
||||||
|
mgcp = mgcp_client_init(ctx, &conf);
|
||||||
|
|
||||||
|
msg = mgcp_msg_gen(mgcp, &mgcp_msg);
|
||||||
|
trans_id = mgcp_msg_trans_id(msg);
|
||||||
|
fprintf(stderr, "- composed msg with trans_id=%u\n", trans_id);
|
||||||
|
|
||||||
|
fprintf(stderr, "- not in queue yet, cannot cancel yet\n");
|
||||||
|
OSMO_ASSERT(mgcp_client_cancel(mgcp, trans_id) == -ENOENT);
|
||||||
|
|
||||||
|
fprintf(stderr, "- enqueue\n");
|
||||||
|
dummy_mgcp_send(msg);
|
||||||
|
|
||||||
|
fprintf(stderr, "- cancel succeeds\n");
|
||||||
|
OSMO_ASSERT(mgcp_client_cancel(mgcp, trans_id) == 0);
|
||||||
|
|
||||||
|
fprintf(stderr, "- late response gets discarded\n");
|
||||||
|
OSMO_ASSERT(reply_to(trans_id, 200, "OK", 1, "v=0\r\n") == -ENOENT);
|
||||||
|
|
||||||
|
fprintf(stderr, "- canceling again does nothing\n");
|
||||||
|
OSMO_ASSERT(mgcp_client_cancel(mgcp, trans_id) == -ENOENT);
|
||||||
|
|
||||||
|
fprintf(stderr, "%s() done\n", __func__);
|
||||||
|
}
|
||||||
|
|
||||||
static const struct log_info_cat log_categories[] = {
|
static const struct log_info_cat log_categories[] = {
|
||||||
};
|
};
|
||||||
|
|
||||||
|
@ -244,10 +290,13 @@ int main(int argc, char **argv)
|
||||||
log_set_use_color(osmo_stderr_target, 0);
|
log_set_use_color(osmo_stderr_target, 0);
|
||||||
log_set_print_category(osmo_stderr_target, 1);
|
log_set_print_category(osmo_stderr_target, 1);
|
||||||
|
|
||||||
|
log_set_category_filter(osmo_stderr_target, DLMGCP, 1, LOGL_DEBUG);
|
||||||
|
|
||||||
mgcp_client_conf_init(&conf);
|
mgcp_client_conf_init(&conf);
|
||||||
|
|
||||||
test_crcx();
|
test_crcx();
|
||||||
test_mgcp_msg();
|
test_mgcp_msg();
|
||||||
|
test_mgcp_client_cancel();
|
||||||
|
|
||||||
printf("Done\n");
|
printf("Done\n");
|
||||||
fprintf(stderr, "Done\n");
|
fprintf(stderr, "Done\n");
|
||||||
|
|
|
@ -1,2 +1,15 @@
|
||||||
DLMGCP message buffer to small, can not generate MGCP message
|
DLMGCP message buffer to small, can not generate MGCP message
|
||||||
|
|
||||||
|
test_mgcp_client_cancel():
|
||||||
|
- composed msg with trans_id=1
|
||||||
|
- not in queue yet, cannot cancel yet
|
||||||
|
DLMGCP Cannot cancel, no such transaction: 1
|
||||||
|
- enqueue
|
||||||
|
- cancel succeeds
|
||||||
|
DLMGCP Canceled transaction 1
|
||||||
|
- late response gets discarded
|
||||||
|
DLMGCP Cannot find matching MGCP transaction for trans_id 1
|
||||||
|
- canceling again does nothing
|
||||||
|
DLMGCP Cannot cancel, no such transaction: 1
|
||||||
|
test_mgcp_client_cancel() done
|
||||||
Done
|
Done
|
||||||
|
|
|
@ -59,4 +59,23 @@ RSIP 5 23@mgw MGCP 1.0
|
||||||
|
|
||||||
Overfolow test:
|
Overfolow test:
|
||||||
|
|
||||||
|
|
||||||
|
test_mgcp_client_cancel():
|
||||||
|
composed:
|
||||||
|
-----
|
||||||
|
CRCX 1 23@mgw MGCP 1.0
|
||||||
|
C: 2f
|
||||||
|
I: 11
|
||||||
|
L: p:20, a:AMR, nt:IN
|
||||||
|
M: sendrecv
|
||||||
|
|
||||||
|
-----
|
||||||
|
composed response:
|
||||||
|
-----
|
||||||
|
200 1 OK
|
||||||
|
I: 1
|
||||||
|
|
||||||
|
v=0
|
||||||
|
|
||||||
|
-----
|
||||||
Done
|
Done
|
||||||
|
|
Loading…
Reference in New Issue