[Asterisk-code-review] avoid merging command and regular T140 text (asterisk[13])

[Richard Mudgett]

Richard Mudgett has posted comments on this change. ( https://gerrit.asterisk.org/9510 )

Change subject: avoid merging command and regular T140 text
......................................................................


Patch Set 1: Code-Review-1

(7 comments)

https://gerrit.asterisk.org/#/c/9510/1//COMMIT_MSG
Commit Message:

https://gerrit.asterisk.org/#/c/9510/1//COMMIT_MSG@7
PS1, Line 7: avoid merging command and regular T140 text
Please use the commit message format as described on the linked wiki page:

https://wiki.asterisk.org/wiki/display/AST/Commit+Messages


https://gerrit.asterisk.org/#/c/9510/1/res/res_rtp_asterisk.c
File res/res_rtp_asterisk.c:

https://gerrit.asterisk.org/#/c/9510/1/res/res_rtp_asterisk.c@6105
PS1, Line 6105: 	if ( !rtp->red ) return 0;
Guidelines:
if (test) {
   return 0;
}

Note spacing around parentheses and use of curly braces.

https://wiki.asterisk.org/wiki/display/AST/Coding+Guidelines


https://gerrit.asterisk.org/#/c/9510/1/res/res_rtp_asterisk.c@6108
PS1, Line 6108: 		const unsigned char * primary = red->buf_data;
Spacing:

const unsigned char *primary = red->buf_data;


https://gerrit.asterisk.org/#/c/9510/1/res/res_rtp_asterisk.c@6110
PS1, Line 6110: 		    (primary[0] == 0x08 || primary[0] == 0x0a || primary[0] == 0x0d)) {
Guidelines: Use tabs to indent not spaces.


https://gerrit.asterisk.org/#/c/9510/1/res/res_rtp_asterisk.c@6113
PS1, Line 6113: 		}
              : 		else {
Guidelines:
if () {
} else {
}


https://gerrit.asterisk.org/#/c/9510/1/res/res_rtp_asterisk.c@6116
PS1, Line 6116: 			if (primary[0] == 0x08 || primary[0] == 0x0a || primary[0] == 0x0d) {
We can read beyond the frame->data.ptr buffer if frame->datalen == 0.  We have only checked if frame->datalen > -1 at this point.


https://gerrit.asterisk.org/#/c/9510/1/res/res_rtp_asterisk.c@6108
PS1, Line 6108: 		const unsigned char * primary = red->buf_data;
              : 		if (red->t140.datalen > 0 &&
              : 		    (primary[0] == 0x08 || primary[0] == 0x0a || primary[0] == 0x0d)) {
              : 			/* flush previous t140 packet if it is a command */
              : 			ast_rtp_write(instance, &rtp->red->t140);
              : 		}
              : 		else {
              : 			primary = frame->data.ptr;
              : 			if (primary[0] == 0x08 || primary[0] == 0x0a || primary[0] == 0x0d) {
              : 				/* flush previous t140 packet we send commands */
              : 				ast_rtp_write(instance, &rtp->red->t140);
              : 			}
              : 		}
I think the code should be this way:

if (red->t140.datalen > 0) {
   const unsigned char *primary = red->buf_data;

   /* There is something already in the T.140 buffer */
   if (primary[0] == 0x08 ....) {
       /* Flush the previous T.140 packet if it is a command */
       ast_rtp_write(instance, &rtp->red->t140);
   } else {
       primary = frame->data->ptr;
       if (primary[0] == 0x08 ....) {
          /* Flush the previous T.140 packet if we are buffering a command now */
          ast_rtp_write(instance, &rtp->red->t140);
       }
   }
}



-- 
To view, visit https://gerrit.asterisk.org/9510
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifbe993311410fa855cb8aa4a12084db75f413462
Gerrit-Change-Number: 9510
Gerrit-PatchSet: 1
Gerrit-Owner: Emmanuel BUU <emmanuel.buu at ives.fr>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Wed, 18 Jul 2018 16:20:59 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180718/9e01b64c/attachment.html>