[asterisk-bugs] [Asterisk 0011797]: app_rtpstream: Application to Page Multicast capable receivers (e.g. Snom, Linksys, Cisco, Barix devices)
noreply at bugs.digium.com
noreply at bugs.digium.com
Wed Jan 30 11:40:31 CST 2008
A NOTE has been added to this issue.
======================================================================
http://bugs.digium.com/view.php?id=11797
======================================================================
Reported By: macbrody
Assigned To:
======================================================================
Project: Asterisk
Issue ID: 11797
Category: Applications/NewFeature
Reproducibility: N/A
Severity: feature
Priority: normal
Status: new
Asterisk Version: SVN
SVN Branch (only for SVN checkouts, not tarball releases): trunk
SVN Revision (number only!): 99188
Disclaimer on File?: N/A
Request Review:
======================================================================
Date Submitted: 01-19-2008 04:46 CST
Last Modified: 01-30-2008 11:40 CST
======================================================================
Summary: app_rtpstream: Application to Page Multicast capable
receivers (e.g. Snom, Linksys, Cisco, Barix devices)
Description:
app_rtpstream is an application that reads the input channel's voice frames
and does rtp stream them to either unicast or multicast addresses defined
as groups in the config file.
This can be used for example with the Snom and Linksys IP Phones' feature
to do paging to multicast receivers.
======================================================================
----------------------------------------------------------------------
oej - 01-30-08 11:40
----------------------------------------------------------------------
Comments made on this code:
------------------------
+ * Information on how multicast paging works with linksys
+ * phones was used from FreeSWITCH's mod_esf with permission
+ * from Brian West.
All references to company names (especially trademarked ones) should be
properly capitalized. There are many places in this file that should be
updated.
+/*** MODULEINFO
+ <defaultenabled>yes</defaultenabled>
+ ***/
This is unnecessary, as the default for modules is 'enabled'.
+
+#define RTP_PT_ULAW 0
+#define RTP_PT_GSM 3
+#define RTP_PT_ALAW 8
+#define RTP_PT_G729 18
This information is already in the static_RTP_PT table in main/rtp.c and
shouldn't be duplicated here. An accessor function that takes an
Asterisk format code and returns the RTP payload type from that table
would provide what is needed here.
+static char *app = "RTPPage";
+static char *synopsis = "RTPPage Application";
Can we have something more description that just repeating the
application's name and adding 'Application'?
+static char *descrip = " RTPPage(direct|multicast,
ip:port[&ip:port]|group[&group2[&group3...]][,codec]): Sends the channel's
input to the\n"
+"specified group(s) defined in the config file rtppage.conf.\n"
+"The optional codec may be one of the following:\n"
+" ulaw - default\n"
+" alaw\n"
+" gsm\n"
+" g729\n"
+"as long as asterisk does not have to translate or respective translators
are\n"
+"installed with your asterisk installation. If none or any other codec
is\n"
+"specified the application will fall back to ulaw.\n";
Why does this not support all formats that are defined in our
static_RTP_PT in main/rtp.c? At a minimum the addition of G.726 and
G.722 could be useful.
+ /* allocate memory for the rtp send buffer */
+ if ((databuf = ast_calloc(1, 172)) == NULL) {
+ ast_log(LOG_WARNING, "Failed to allocate memory for the data buffer,
give up\n");
+ goto end;
+ }
This makes an assumption about the amount of data to be sent... see
below.
+
+ /* initialize rtp buffer header
+ * with rtp version and
+ * payload type
+ */
+ rtph = (struct rtp_header *)databuf;
I would prefer to see this buffer just allocated as a 'struct
rtp_header' instance and define that structure to contain a 'char
data[0]' element at its end for reading and writing the payload portion
of the frame.
+ rtpflags = (0x02 << 14); /* rtp v2 */
+ rtpflags = (rtpflags & 0xFF80) | rtp_pt;
+ rtph->flags = htons(rtpflags);
+ rtph->ssrc = htonl((u_long)time(NULL));
+
+ /* first create a temporary table for this page session
+ * containing all groups which will be used
+ */
+ AST_LIST_LOCK(&groups);
+ rest = ast_strdup(args.groups);
There is no need to duplicate this string, as the one in args.groups is
already modifiable and no other usage of the original string is
necessary. In addition, this allocation is never freed and couldn't be
because the pointer to the base of the copied string is not preserved
anywhere.
+ struct mcast_group *agroup = ast_calloc(1, sizeof(*agroup));
No check for allocation failure.
+ rest2 = ast_strdup(cur);
Same comment as above; there is no need for a copy, and this is a memory
leak.
+ struct mcast_group *agroup = ast_calloc(1, sizeof(*agroup));
No check for allocation failure.
+ memcpy(agroup->name, group->name, 32);
+ agroup->type = group->type;
+ agroup->socket = group->socket;
+ agroup->ttl = group->ttl;
+ memcpy(&agroup->rtp_address, &group->rtp_address,
sizeof(agroup->rtp_address));
+ memcpy(&agroup->control_address, &group->control_address,
sizeof(agroup->control_address));
If we are going to copy the entire group, then just copy the whole group
using '*agroup = *group;' and then 'AST_LIST_NEXT(agroup, list) = NULL;'.
+ AST_LIST_TRAVERSE(&activegroups, group, list) {
+ group->socket = socket(AF_INET, SOCK_DGRAM, 0);
No check for socket creation failure.
+ memcpy(&destaddr, &group->control_address, sizeof(destaddr));
'destaddr = group->control_address;'
+ sendto(group->socket, &cpk, sizeof(cpk), 0, (struct sockaddr
*)&destaddr, sizeof(destaddr));
+ sendto(group->socket, &cpk, sizeof(cpk), 0, (struct sockaddr
*)&destaddr, sizeof(destaddr));
If there is a specific reason why this packet is being sent twice, it
would be helpful if it was noted in a comment here.
+ ast_log(LOG_DEBUG, "Hangup detected\n");
Please ast_debug() and ast_verb() for log messages.
+ if ((f->frametype == AST_FRAME_DTMF) && (f->subclass == '#')) {
+ res = 0;
+ ast_frfree(f);
+ ast_log(LOG_DEBUG, "Received DTMF key: %d\n", f->subclass);
You can't access f->subclass when you just freed the frame with
ast_frfree().
+ memcpy(databuf+12, f->data, f->datalen);
This is very bad. This will copy an unpredictable amount of data into a
fixed-size buffer. The channel running this application could supply
voice frames with any number of samples it wishes. Either this code will
need to grow the size of the databuffer to match the largest incoming
frame it has seen, or it will need to use a smoother to intentionally
read known-size frames from the incoming channel (although that won't
work for G.729 and G.723 because they are not smoothable in the same way
that sample-based codecs are).
+ memcpy(&destaddr, &group->rtp_address, sizeof(destaddr));
Again a direct structure copy can be used.
+ if (sendto(group->socket, databuf, f->datalen+12, 0, (struct sockaddr
*)&destaddr, sizeof(destaddr)) <= 0) {
+ ast_log(LOG_DEBUG, "sendto() failed!\n");
+ }
If the sendto() fails, we don't get a useful error message (including
the translated error text, and no attempt is made to close the socket if
it is no longer usable, so the console will be spammed with failure
messages for each incoming frame. In addition, a serious failure like
this should not be LOG_DEBUG.
+ AST_LIST_LOCK(&groups);
+ if (reload) {
+ /* if this is a reload, then free the config structure before
+ * filling it again
+ */
+ while ((group = AST_LIST_REMOVE_HEAD(&groups, list))) {
+ free(group);
+ }
+
+ /* reset default_ttl & tos */
+ default_ttl = -1; /* means not set */
+ tos = -1;
+ }
The actual paging code accesses default_ttl and tos outside the 'groups'
lock, so these changes could affect existing paging calls that are being
setup during the reload.
+ return(-1);
Don't treat return as if it was a function :-)
+ var = ast_variable_retrieve(cfg, cat, "tos");
+ if (var) {
+ ast_str2tos(var, &tos);
+ }
No error check to see if the supplied TOS string was invalid.
+ group = ast_calloc(1, sizeof(*group));
No check for allocation failure.
+ memset(&group->rtp_address, 0, sizeof(group->rtp_address));
This is unnecessary, as the memory for the structure was allocated with
ast_calloc().
+ group->rtp_address.sin_port = htons(atoi(ast_variable_retrieve(cfg,
cat, "rtp_port")));
If no rtp_port was specified, this will segfault.
+ if (inet_pton(AF_INET, ast_variable_retrieve(cfg, cat, "rtp_address"),
&group->rtp_address.sin_addr) <= 0) {
Same thing here... no rtp_address specified, segfault.
+ group->type = -1;
+ group->socket = -1;
+ group->ttl = -1;
+ ast_log(LOG_NOTICE, "Invalid mcast group %s!\n", cat);
+ continue;
Memory leak. The next cycle through this loop will allocate another
group structure. There is no need to reset the fields in this one, it
should just be freed before the log message. Also, configuration errors
should be at least LOG_WARNING, if not LOG_ERROR.
+ ast_log(LOG_NOTICE, "loaded category %s\n", group->name);
This is excessive and seems like a leftover debugging message from
development.
Issue History
Date Modified Username Field Change
======================================================================
01-30-08 11:40 oej Note Added: 0081449
======================================================================
More information about the asterisk-bugs
mailing list