<p>Richard Mudgett <strong>posted comments</strong> on this change.</p><p><a href="https://gerrit.asterisk.org/8762">View Change</a></p><p>Patch set 2:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p>(27 comments)</p><ul style="list-style: none; padding-left: 20px;"><li><p><a href="https://gerrit.asterisk.org/#/c/8762/2/bridges/bridge_softmix.c">File bridges/bridge_softmix.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8762/2/bridges/bridge_softmix.c@738">Patch Set #2, Line 738:</a> <code style="font-family:monospace,monospace"> struct ast_msg_data *msg = frame->data.ptr;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">If an AST_FRAME_TEXT came in then the text string would be interpreted as an ast_msg_data struct which is bad.</p><p style="white-space: pre-wrap; word-wrap: break-word;">You'll have to have different ast_log() messages to handle the different text frame formats.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8762/2/bridges/bridge_softmix.c@743">Patch Set #2, Line 743:</a> <code style="font-family:monospace,monospace"> ast_debug(1, "Received %s frame from '%s:%s': %s\n", frame_type,</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Just ast_log(LOG_DEBUG,... because you have already decided to log something.</p><p style="white-space: pre-wrap; word-wrap: break-word;">ast_debug() includes the DEBUG_ATLEAST test so the built in test is redundant.</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/8762/2/channels/chan_pjsip.c">File channels/chan_pjsip.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8762/2/channels/chan_pjsip.c@2272">Patch Set #2, Line 2272:</a> <code style="font-family:monospace,monospace"> ao2_ref(data->session, -1);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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().</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8762/2/channels/chan_pjsip.c@2417">Patch Set #2, Line 2417:</a> <code style="font-family:monospace,monospace"> msg = ast_msg_data_alloc(AST_MSG_DATA_SOURCE_TYPE_UNKNOWN, attrs, ARRAY_LEN(attrs));</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">ast_msg_data_dup() checks w/ assert.<br>ast_msg_data_get_attribute() checks w/ assert.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Either add the check here or remove the asserts.</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/8762/2/include/asterisk/channel.h">File include/asterisk/channel.h:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8762/2/include/asterisk/channel.h@130">Patch Set #2, Line 130:</a> <code style="font-family:monospace,monospace">#include "asterisk/message.h"</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/8762/2/include/asterisk/frame.h">File include/asterisk/frame.h:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8762/2/include/asterisk/frame.h@131">Patch Set #2, Line 131:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> /*! Text message in an ast_msg_data structure */<br> AST_FRAME_TEXT_DATA,<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/8762/2/main/bridge_channel.c">File main/bridge_channel.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8762/2/main/bridge_channel.c@1004">Patch Set #2, Line 1004:</a> <code style="font-family:monospace,monospace"> ast_debug(1, "Queuing TEXT frame to '%s': %*.s\n", ast_channel_name(bridge_channel->chan),</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">ast_log(LOG_DEBUG,...</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8762/2/main/bridge_channel.c@1008">Patch Set #2, Line 1008:</a> <code style="font-family:monospace,monospace"> ast_debug(1, "Sending TEXT_DATA frame from '%s' to '%s:%s': %s\n",</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">ast_log(LOG_DEBUG,...</p><p style="white-space: pre-wrap; word-wrap: break-word;">Also s/Sending/Queuing/</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8762/2/main/bridge_channel.c@2370">Patch Set #2, Line 2370:</a> <code style="font-family:monospace,monospace"> if (DEBUG_ATLEAST(1)) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">No need for DEBUG_ATLEAST which is redundant with ast_debug().</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/8762/2/main/channel.c">File main/channel.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8762/2/main/channel.c@4878">Patch Set #2, Line 4878:</a> <code style="font-family:monospace,monospace"> size_t body_len = strlen(body) + 1;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">f.datalen = strlen(body);</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8762/2/main/channel.c@4888">Patch Set #2, Line 4888:</a> <code style="font-family:monospace,monospace"> if (ast_channel_tech(chan)->write_text</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8762/2/main/channel.c@4897">Patch Set #2, Line 4897:</a> <code style="font-family:monospace,monospace"> f.datalen = body_len;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Probably should not send empty text messages over T.140.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8762/2/main/channel.c@4898">Patch Set #2, Line 4898:</a> <code style="font-family:monospace,monospace"> f.data.ptr = ast_strdup(body);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">You lost the allocation NULL check when you "moved" the code from ast_sendtext().</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8762/2/main/channel.c@4916">Patch Set #2, Line 4916:</a> <code style="font-family:monospace,monospace"> ast_debug(1, "Unable to send text message on channel '%s'\n", ast_channel_name(chan));</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">"Channel technology does not support sending text on channel '%s'\n"</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8762/2/main/channel.c@4932">Patch Set #2, Line 4932:</a> <code style="font-family:monospace,monospace"> .value = (char *)text,</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Replace text with:<br>S_OR(text, "")</p><p style="white-space: pre-wrap; word-wrap: break-word;">Otherwise you will hit an abort/assert in ast_msg_data_alloc() if text is NULL.</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/8762/2/main/message.c">File main/message.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8762/2/main/message.c@1430">Patch Set #2, Line 1430:</a> <code style="font-family:monospace,monospace"> dest = ast_calloc(1, msg->length);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Use ast_malloc() instead since you are immediately setting every byte with the memcpy().</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8762/2/main/message.c@1490">Patch Set #2, Line 1490:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> f.subclass.integer = 0;<br> f.offset = 0;<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">memset already set these to zero.</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/8762/2/res/res_pjsip_messaging.c">File res/res_pjsip_messaging.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8762/2/res/res_pjsip_messaging.c@64">Patch Set #2, Line 64:</a> <code style="font-family:monospace,monospace">static enum pjsip_status_code check_content_type(const pjsip_rx_data *rdata)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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().</p><p style="white-space: pre-wrap; word-wrap: break-word;">Also you did not update the function doxygen comment.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8762/2/res/res_pjsip_messaging.c@770">Patch Set #2, Line 770:</a> <code style="font-family:monospace,monospace"> struct ast_msg_data_attribute attrs[4];</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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().</p><p style="white-space: pre-wrap; word-wrap: break-word;">As an example:</p><p style="white-space: pre-wrap; word-wrap: break-word;">int pos = 0;</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">if we have a from value {<br> attrs[pos].type = AST_MSG_DATA_ATTR_FROM;<br> attrs[pos].value = "from string";<br> ++pos;<br>}</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">if we have a to value {<br> attrs[pos].type = AST_MSG_DATA_ATTR_TO;<br> attrs[pos].value = "to string";<br> ++pos;<br>}</pre><p style="white-space: pre-wrap; word-wrap: break-word;">attrs[pos].type = AST_MSG_DATA_ATTR_CONTENT_TYPE;<br>attrs[pos].value = "text/plain";<br>++pos;</p><p style="white-space: pre-wrap; word-wrap: break-word;">attrs[pos].type = AST_MSG_DATA_ATTR_BODY;<br>attrs[pos].value = "Text body contents";<br>++pos;</p><p style="white-space: pre-wrap; word-wrap: break-word;">msg = ast_msg_data_alloc(AST_MSG_DATA_SOURCE_IN_DIALOG, attrs, pos);</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8762/2/res/res_pjsip_messaging.c@790">Patch Set #2, Line 790:</a> <code style="font-family:monospace,monospace"> attrs[0].value = ast_alloca(from_len);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">ast_alloca can never fail. Otherwise we blew the stack and crashed.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8762/2/res/res_pjsip_messaging.c@797">Patch Set #2, Line 797:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> if (!attrs[0].value) {<br> send_response(rdata, PJSIP_SC_INTERNAL_SERVER_ERROR, dlg, tsx);<br> return 0;<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think failing because there is no display name available is wrong.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Also attrs[0].value could be uninitialized!</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8762/2/res/res_pjsip_messaging.c@805">Patch Set #2, Line 805:</a> <code style="font-family:monospace,monospace"> if (to_len) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">If we have no To display name then attrs[1].value is uninitialized.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8762/2/res/res_pjsip_messaging.c@807">Patch Set #2, Line 807:</a> <code style="font-family:monospace,monospace"> attrs[1].value = ast_alloca(to_len);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">same</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8762/2/res/res_pjsip_messaging.c@814">Patch Set #2, Line 814:</a> <code style="font-family:monospace,monospace"> attrs[2].value = ast_alloca(rdata->msg_info.msg->body->content_type.type.slen</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">same</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8762/2/res/res_pjsip_messaging.c@827">Patch Set #2, Line 827:</a> <code style="font-family:monospace,monospace"> attrs[3].value = ast_alloca(rdata->msg_info.msg->body->len + 1);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">same</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8762/2/res/res_pjsip_messaging.c@832">Patch Set #2, Line 832:</a> <code style="font-family:monospace,monospace"> ast_copy_string(attrs[3].value, rdata->msg_info.msg->body->data, rdata->msg_info.msg->body->len);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.)</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/8762/2/res/res_pjsip_messaging.c@840">Patch Set #2, Line 840:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> ast_debug(1, "Received in-dialog MESSAGE from '%s:%s': %s %s\n",<br> attrs[0].value, ast_channel_name(session->channel),<br> attrs[1].value, attrs[2].value);<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Use ast_msg_data_get_attribute() instead of hard coded attrs[] index values. The from and to values may not exist.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/8762">change 8762</a>. To unsubscribe, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/8762"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 13 </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: Idacf5900bfd5f22ab8cd235aa56dfad090d18489 </div>
<div style="display:none"> Gerrit-Change-Number: 8762 </div>
<div style="display:none"> Gerrit-PatchSet: 2 </div>
<div style="display:none"> Gerrit-Owner: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 </div>
<div style="display:none"> Gerrit-Reviewer: Richard Mudgett <rmudgett@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Thu, 12 Apr 2018 20:42:29 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>