[Asterisk-code-review] bridge softmix: Forward TEXT frames (asterisk[13])
Richard Mudgett
asteriskteam at digium.com
Thu Apr 12 15:42:29 CDT 2018
Richard Mudgett has posted comments on this change. ( https://gerrit.asterisk.org/8762 )
Change subject: bridge_softmix: Forward TEXT frames
......................................................................
Patch Set 2: Code-Review-1
(27 comments)
https://gerrit.asterisk.org/#/c/8762/2/bridges/bridge_softmix.c
File bridges/bridge_softmix.c:
https://gerrit.asterisk.org/#/c/8762/2/bridges/bridge_softmix.c@738
PS2, Line 738: struct ast_msg_data *msg = frame->data.ptr;
If an AST_FRAME_TEXT came in then the text string would be interpreted as an ast_msg_data struct which is bad.
You'll have to have different ast_log() messages to handle the different text frame formats.
https://gerrit.asterisk.org/#/c/8762/2/bridges/bridge_softmix.c@743
PS2, Line 743: ast_debug(1, "Received %s frame from '%s:%s': %s\n", frame_type,
Just ast_log(LOG_DEBUG,... because you have already decided to log something.
ast_debug() includes the DEBUG_ATLEAST test so the built in test is redundant.
https://gerrit.asterisk.org/#/c/8762/2/channels/chan_pjsip.c
File channels/chan_pjsip.c:
https://gerrit.asterisk.org/#/c/8762/2/channels/chan_pjsip.c@2272
PS2, Line 2272: ao2_ref(data->session, -1);
This needs to be an ao2_cleanup() since it is a destructor. Also you now have a path where it can be NULL in sendtext_data_create().
https://gerrit.asterisk.org/#/c/8762/2/channels/chan_pjsip.c@2417
PS2, Line 2417: msg = ast_msg_data_alloc(AST_MSG_DATA_SOURCE_TYPE_UNKNOWN, attrs, ARRAY_LEN(attrs));
You should check for NULL return. Otherwise you are depending upon chan_pjsip_sendtext_data() internal calls to check for you and those checks include asserts as part of the check.
ast_msg_data_dup() checks w/ assert.
ast_msg_data_get_attribute() checks w/ assert.
Either add the check here or remove the asserts.
https://gerrit.asterisk.org/#/c/8762/2/include/asterisk/channel.h
File include/asterisk/channel.h:
https://gerrit.asterisk.org/#/c/8762/2/include/asterisk/channel.h@130
PS2, Line 130: #include "asterisk/message.h"
Rather than making everyone dependent on this file when they include channel.h just forward declare struct ast_msg_data instead. This is what message.h does since the struct is opaque.
https://gerrit.asterisk.org/#/c/8762/2/include/asterisk/frame.h
File include/asterisk/frame.h:
https://gerrit.asterisk.org/#/c/8762/2/include/asterisk/frame.h@131
PS2, Line 131: /*! Text message in an ast_msg_data structure */
: AST_FRAME_TEXT_DATA,
For v13 you MUST leave a space for AST_FRAME_RTCP after AST_FRAME_BRIDGE_ACTION_SYNC. Either that or define it. See the doxygen comment about IAX2 above.
https://gerrit.asterisk.org/#/c/8762/2/main/bridge_channel.c
File main/bridge_channel.c:
https://gerrit.asterisk.org/#/c/8762/2/main/bridge_channel.c@1004
PS2, Line 1004: ast_debug(1, "Queuing TEXT frame to '%s': %*.s\n", ast_channel_name(bridge_channel->chan),
ast_log(LOG_DEBUG,...
https://gerrit.asterisk.org/#/c/8762/2/main/bridge_channel.c@1008
PS2, Line 1008: ast_debug(1, "Sending TEXT_DATA frame from '%s' to '%s:%s': %s\n",
ast_log(LOG_DEBUG,...
Also s/Sending/Queuing/
https://gerrit.asterisk.org/#/c/8762/2/main/bridge_channel.c@2370
PS2, Line 2370: if (DEBUG_ATLEAST(1)) {
No need for DEBUG_ATLEAST which is redundant with ast_debug().
https://gerrit.asterisk.org/#/c/8762/2/main/channel.c
File main/channel.c:
https://gerrit.asterisk.org/#/c/8762/2/main/channel.c@4878
PS2, Line 4878: size_t body_len = strlen(body) + 1;
This is only used by the T.140 section so it should be moved there. The T.140 code did not include the null terminator in the length.
f.datalen = strlen(body);
https://gerrit.asterisk.org/#/c/8762/2/main/channel.c@4888
PS2, Line 4888: if (ast_channel_tech(chan)->write_text
For channel drivers that only support T.140 or the old send_text callback we should discard sending any messages that are not text/plain content and report failure.
https://gerrit.asterisk.org/#/c/8762/2/main/channel.c@4897
PS2, Line 4897: f.datalen = body_len;
Probably should not send empty text messages over T.140.
https://gerrit.asterisk.org/#/c/8762/2/main/channel.c@4898
PS2, Line 4898: f.data.ptr = ast_strdup(body);
You lost the allocation NULL check when you "moved" the code from ast_sendtext().
https://gerrit.asterisk.org/#/c/8762/2/main/channel.c@4916
PS2, Line 4916: ast_debug(1, "Unable to send text message on channel '%s'\n", ast_channel_name(chan));
"Channel technology does not support sending text on channel '%s'\n"
https://gerrit.asterisk.org/#/c/8762/2/main/channel.c@4932
PS2, Line 4932: .value = (char *)text,
Replace text with:
S_OR(text, "")
Otherwise you will hit an abort/assert in ast_msg_data_alloc() if text is NULL.
https://gerrit.asterisk.org/#/c/8762/2/main/message.c
File main/message.c:
https://gerrit.asterisk.org/#/c/8762/2/main/message.c@1430
PS2, Line 1430: dest = ast_calloc(1, msg->length);
Use ast_malloc() instead since you are immediately setting every byte with the memcpy().
https://gerrit.asterisk.org/#/c/8762/2/main/message.c@1490
PS2, Line 1490: f.subclass.integer = 0;
: f.offset = 0;
memset already set these to zero.
https://gerrit.asterisk.org/#/c/8762/2/res/res_pjsip_messaging.c
File res/res_pjsip_messaging.c:
https://gerrit.asterisk.org/#/c/8762/2/res/res_pjsip_messaging.c@64
PS2, Line 64: static enum pjsip_status_code check_content_type(const pjsip_rx_data *rdata)
This allows any MESSAGE content type even for out-of-dialog MESSAGEs. We still only send out-of-dialog text/plain content so we need the old version for module_on_rx_request() and this new version for incoming_in_dialog_request().
Also you did not update the function doxygen comment.
https://gerrit.asterisk.org/#/c/8762/2/res/res_pjsip_messaging.c@770
PS2, Line 770: struct ast_msg_data_attribute attrs[4];
Since the From and To display values are optional you probably do not want to add them to attrs[]. Have a pos index that you use instead to put values into the attrs[] array. Once you have all available values in the array then pos is the array size you pass to ast_msg_data_alloc().
As an example:
int pos = 0;
if we have a from value {
attrs[pos].type = AST_MSG_DATA_ATTR_FROM;
attrs[pos].value = "from string";
++pos;
}
if we have a to value {
attrs[pos].type = AST_MSG_DATA_ATTR_TO;
attrs[pos].value = "to string";
++pos;
}
attrs[pos].type = AST_MSG_DATA_ATTR_CONTENT_TYPE;
attrs[pos].value = "text/plain";
++pos;
attrs[pos].type = AST_MSG_DATA_ATTR_BODY;
attrs[pos].value = "Text body contents";
++pos;
msg = ast_msg_data_alloc(AST_MSG_DATA_SOURCE_IN_DIALOG, attrs, pos);
https://gerrit.asterisk.org/#/c/8762/2/res/res_pjsip_messaging.c@790
PS2, Line 790: attrs[0].value = ast_alloca(from_len);
ast_alloca can never fail. Otherwise we blew the stack and crashed.
https://gerrit.asterisk.org/#/c/8762/2/res/res_pjsip_messaging.c@797
PS2, Line 797: if (!attrs[0].value) {
: send_response(rdata, PJSIP_SC_INTERNAL_SERVER_ERROR, dlg, tsx);
: return 0;
: }
I think failing because there is no display name available is wrong.
Also attrs[0].value could be uninitialized!
https://gerrit.asterisk.org/#/c/8762/2/res/res_pjsip_messaging.c@805
PS2, Line 805: if (to_len) {
If we have no To display name then attrs[1].value is uninitialized.
https://gerrit.asterisk.org/#/c/8762/2/res/res_pjsip_messaging.c@807
PS2, Line 807: attrs[1].value = ast_alloca(to_len);
same
https://gerrit.asterisk.org/#/c/8762/2/res/res_pjsip_messaging.c@814
PS2, Line 814: attrs[2].value = ast_alloca(rdata->msg_info.msg->body->content_type.type.slen
same
https://gerrit.asterisk.org/#/c/8762/2/res/res_pjsip_messaging.c@827
PS2, Line 827: attrs[3].value = ast_alloca(rdata->msg_info.msg->body->len + 1);
same
https://gerrit.asterisk.org/#/c/8762/2/res/res_pjsip_messaging.c@832
PS2, Line 832: ast_copy_string(attrs[3].value, rdata->msg_info.msg->body->data, rdata->msg_info.msg->body->len);
We copy the message body to the stack. We probably should have an upper limit on the body size to protect ourself from blowing the stack. This is something you have removed with this change. (The MAX_BODY_SIZE check.)
https://gerrit.asterisk.org/#/c/8762/2/res/res_pjsip_messaging.c@840
PS2, Line 840: ast_debug(1, "Received in-dialog MESSAGE from '%s:%s': %s %s\n",
: attrs[0].value, ast_channel_name(session->channel),
: attrs[1].value, attrs[2].value);
Use ast_msg_data_get_attribute() instead of hard coded attrs[] index values. The from and to values may not exist.
--
To view, visit https://gerrit.asterisk.org/8762
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-MessageType: comment
Gerrit-Change-Id: Idacf5900bfd5f22ab8cd235aa56dfad090d18489
Gerrit-Change-Number: 8762
Gerrit-PatchSet: 2
Gerrit-Owner: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Thu, 12 Apr 2018 20:42:29 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180412/8f20f684/attachment-0001.html>
More information about the asterisk-code-review
mailing list