> On April 8, 2015, 5:05 p.m., Mark Michelson wrote:
> > I have a few problems with this review:
> >
> > * I don't see in this review or in the patch you uploaded to the linked
> > issue where ast_escape_c() or astman_escape_output() are defined.
> > * This patch performs a lot of heap allocations and does not check for
> > potential memory allocation failure.
> > * This patch places the responsibility of escaping strings on all callers
> > of ast_manager_event(). If escaping strings is an essential part of sending
> > manager events, then it should be built into the function that sends
> > manager events. Otherwise, it's easy for a programmer to forget to perform
> > the escaping before outputting strings. If escaping of manager events is in
> > the same location where manager events are sent, you may also be able to
> > get away with fewer heap allocations, too, which is another bonus.
The patch was missing a bunch of the code from line ending problems, it should
all be there now.
I very much agree with your third point. My initial attempt was to add a
format string parameter for %M to escape manager output, such as:
manager_event(SOME_EVENT_FLAG, "EventName",
"KeyOne: %M\r\nKeyTwo: %s\r\n", val1, val2);
Which takes care of the allocation and everything. It works well, however you
can't tell gcc about added format params at compile time (that I know of at
least), and since -Wformat is used anytime the %M shows up it throws a warning,
which makes it unusable.
What do you think about changing the manager event handing so that it always
escapes %s (using the formatter like %M), and then having something like %S for
an unescaped string? This would leave all the changes inside of manager and
the only applications that would have to be changed are the very few that add
dynamic event keys like:
manager_event(SOME_EVENT_FLAG, "EventName",
"KeyOne: %M\r\nKeyTwo: %s\r\n%s", val1, val2, eventstring);
I would update the documentation for the manager_event functions to let people
know %s is escaped, and %S is unescaped - but it would make it so that it would
be done correctly by default. It shouldn't throw the compile time warnings
either since it would be done in the va_start() / va_end() section of
main/manager.c's __ast_manager_event_multichan(), however if someone were to
use an unescaped string of %S it would still throw a warning...
- warren
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4594/#review15129
-----------------------------------------------------------
On April 8, 2015, 5:51 p.m., warren smith wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4594/
> -----------------------------------------------------------
>
> (Updated April 8, 2015, 5:51 p.m.)
>
>
> Review request for Asterisk Developers.
>
>
> Bugs: asterisk-24934
> https://issues.asterisk.org/jira/browse/asterisk-24934
>
>
> Repository: Asterisk
>
>
> Description
> -------
>
> Asterisk manager output is created using printf formatting, like:
>
> manager_event(SOME_EVENT_FLAG, "EventName",
> "KeyOne: %s\r\nKeyTwo: %s\r\n", val1, val2);
>
> This causes problems when the values themselves contain control characters
> like carriage return and newline, so that applications parsing the output
> will interpret this as a new key, or the end of an event. An example of this
> is having a callerid contain "\r\n\r\n". This ends the event, and the keys
> for the same event are interpreted as a new message, and any keys below are
> missed for the real event.
>
> I've included a patch that provides a ast_escape_c() function which takes a
> string, then returns a pointer to a new string that has the c characters
> escaped (i.e., newline into \n). I've modified the calls to the
> manager_event functions (manager_event, ast_manager_event,
> ast_manager_event_multichan) so that values that could be set by a user are
> escaped. The string values that as far as I know aren't user-created were
> left as-is, like channel names and uniqueid.
>
> There are quite a few calls to the manager event functions and I've double
> checked to make sure all memory allocations are freed after creating the
> escaped string. I also had added an ast_replace_string function which i
> didn't end up using, and added an ast_escape_output function which just calls
> ast_escape_c. An alternative would be to replace the sequence "\r\n" with
> the escaped version, rather than the individual characters.
>
> I'm testing on our asterisk 11 install and this fixes the parsing bugs we run
> into from messed up callerids and things like agent names containing return +
> newline.
>
>
> Diffs
> -----
>
> http://svn.asterisk.org/svn/asterisk/branches/11/res/res_xmpp.c 433419
> http://svn.asterisk.org/svn/asterisk/branches/11/res/res_musiconhold.c
> 433419
> http://svn.asterisk.org/svn/asterisk/branches/11/res/res_fax.c 433419
> http://svn.asterisk.org/svn/asterisk/branches/11/res/res_agi.c 433419
> http://svn.asterisk.org/svn/asterisk/branches/11/pbx/pbx_realtime.c 433419
> http://svn.asterisk.org/svn/asterisk/branches/11/main/utils.c 433419
> http://svn.asterisk.org/svn/asterisk/branches/11/main/test.c 433419
> http://svn.asterisk.org/svn/asterisk/branches/11/main/pbx.c 433419
> http://svn.asterisk.org/svn/asterisk/branches/11/main/manager.c 433419
> http://svn.asterisk.org/svn/asterisk/branches/11/main/features.c 433419
> http://svn.asterisk.org/svn/asterisk/branches/11/main/channel.c 433419
> http://svn.asterisk.org/svn/asterisk/branches/11/main/cdr.c 433419
> http://svn.asterisk.org/svn/asterisk/branches/11/include/asterisk/utils.h
> 433419
> http://svn.asterisk.org/svn/asterisk/branches/11/include/asterisk/strings.h
> 433419
>
> http://svn.asterisk.org/svn/asterisk/branches/11/include/asterisk/presencestate.h
> 433419
> http://svn.asterisk.org/svn/asterisk/branches/11/include/asterisk/manager.h
> 433419
> http://svn.asterisk.org/svn/asterisk/branches/11/funcs/func_presencestate.c
> 433419
> http://svn.asterisk.org/svn/asterisk/branches/11/funcs/func_global.c 433419
> http://svn.asterisk.org/svn/asterisk/branches/11/channels/chan_sip.c 433419
> http://svn.asterisk.org/svn/asterisk/branches/11/channels/chan_local.c
> 433419
> http://svn.asterisk.org/svn/asterisk/branches/11/channels/chan_iax2.c
> 433419
> http://svn.asterisk.org/svn/asterisk/branches/11/channels/chan_agent.c
> 433419
> http://svn.asterisk.org/svn/asterisk/branches/11/cel/cel_manager.c 433419
> http://svn.asterisk.org/svn/asterisk/branches/11/cdr/cdr_manager.c 433419
> http://svn.asterisk.org/svn/asterisk/branches/11/apps/app_voicemail.c
> 433419
> http://svn.asterisk.org/svn/asterisk/branches/11/apps/app_userevent.c
> 433419
> http://svn.asterisk.org/svn/asterisk/branches/11/apps/app_stack.c 433419
> http://svn.asterisk.org/svn/asterisk/branches/11/apps/app_queue.c 433419
> http://svn.asterisk.org/svn/asterisk/branches/11/apps/app_meetme.c 433419
> http://svn.asterisk.org/svn/asterisk/branches/11/apps/app_dial.c 433419
> http://svn.asterisk.org/svn/asterisk/branches/11/apps/app_confbridge.c
> 433419
>
> Diff: https://reviewboard.asterisk.org/r/4594/diff/
>
>
> Testing
> -------
>
> In our production environment I have been running the patch for about 5 days.
> The parsing issues we have had in the past are now resolved.
>
>
> Thanks,
>
> warren smith
>
>
--
_____________________________________________________________________
-- Bandwidth and Colocation Provided by http://www.api-digital.com --
asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
http://lists.digium.com/mailman/listinfo/asterisk-dev
