[asterisk-dev] [Code Review]: app_queue log membername when state_interface is set for ADDMEMBER and REMOVEMEMBER events.

[jamuel]

On July 12, 2011, 1:42 p.m., jamuel wrote:
> > The added if()s alternate between checking for NULL and using ast_strlen_zero.  ast_strlen_zero would probably be more appropriate in all cases.
> 
> jamuel wrote:
>     Testing for ast_strlen_zero in all instances can't be done since mem can be NULL and ast_strlen_zero will segfault if it tests deferencing a structure that is initialized as NULL.
> 
> Tilghman Lesher wrote:
>     Yes, it can be used, and no, it won't segfault.  ast_strlen_zero() returns true if the value passed is itself NULL.
> 
> jamuel wrote:
>     In rqm_exec (line 5735), tried testing with ast_strlen_zero(mem->membername) instead of mem->membername == NULL and I get a segfault:
>     
>     Breakpoint 1, rqm_exec (chan=0xf502d30, data=0xb7395c08 "4152000400,Local/4152000501 at from-internal/n") at app_queue.c:5735
>     5735                    if (ast_strlen_zero(mem->membername) || !log_membername_as_agent) {
>     (gdb) step
>     53              if (!s || (*s == '\0')) {
>     (gdb) p mem
>     $1 = (struct member *) 0x0
>     (gdb) step
>     
>     Program received signal SIGSEGV, Segmentation fault.
>     0x00c72627 in rqm_exec (chan=0xf502d30, data=0xb7395c08 "4152000400,Local/4152000501 at from-internal/n") at /usr/local/src/asterisk-TRUNK/include/asterisk/strings.h:53
>     53              if (!s || (*s == '\0')) {
>     
>     I must be doing something stupid.  Any help would be greatly appreciated.
> 
> opticron wrote:
>     The danger of segfault is with mem being NULL, not mem->membername.  If you test log_membername_as_agent first, it's less likely to happen, but still possible.  You'll want to ensure that mem is not NULL before dereferencing it.

So without complicating the test any further I'll leave the test as mem == NULL--this should always be safe and eliminate the potential for segfault corner-cases.


- jamuel


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/1286/#review3858
-----------------------------------------------------------


On July 12, 2011, 5:12 p.m., jamuel wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1286/
> -----------------------------------------------------------
> 
> (Updated July 12, 2011, 5:12 p.m.)
> 
> 
> Review request for Asterisk Developers, Tilghman Lesher, leifmadsen, opticron, and irroot.
> 
> 
> Summary
> -------
> 
> app_queue logs the events ADDMEMBER and REMOVEMEMBER with the agent field set to the interface value rather than the membername value when a member is added with a state_interface value set.  However all other member related queue events are logged with the membername when a state_interface is set. It would seem that these ADD/REMOVE MEMBER events should log the agent field in the same manner. 
> 
> This patch addresses the original issue reported in https://issues.asterisk.org/jira/browse/ASTERISK-14769 by providing a queues.conf general configuration option log_membername_as-agent. Setting log_membername_as_agent = yes will cause the membername to be logged in the agent field for ADDMEMBER and REMOVEMEMBER queue events if a state_interface has been set.
> 
> The default value (log_membername_as_agent = no) does not alter the existing logging for the ADDMEMBER and REMOVEMEMBER events to maintain existing (buggy) behavior for the sake of backwards compatibility. 
> 
> 
> This addresses bug https://issues.asterisk.org/jira/browse/ASTERISK-14769.
>     https://issues.asterisk.org/jira/browse/https://issues.asterisk.org/jira/browse/ASTERISK-14769
> 
> 
> Diffs
> -----
> 
>   http://svn.asterisk.org/svn/asterisk/trunk/apps/app_queue.c 327949 
>   http://svn.asterisk.org/svn/asterisk/trunk/configs/queues.conf.sample 327949 
> 
> Diff: https://reviewboard.asterisk.org/r/1286/diff
> 
> 
> Testing
> -------
> 
> Tested add, remove, pause, unpause via AMI, CLI, and Dialplan apps and see the correct log output to queue_log.  Also tested with log_membername_as_agent omitted to insure backward compatibility for those queues.conf that might not be updated for this new option. 
> 
> Sample test plan (add and remove)
> ---------------------------------
> 
> set log_membername_as_agent = yes in queues.conf
> 
> AMI:
> ====
> Action: QueueAdd
> Queue: 4152000400
> Interface: Local/4152000501 at from-internal/n
> Penalty: 0
> Paused: 0
> MemberName: Edward Frank
> 
> Action: QueueRemove
> Queue: 4152000400
> Interface: Local/4152000501 at from-internal/n
> 
> 
> Action: QueueAdd
> Queue: 4152000400
> Interface: Local/4152000501 at from-internal/n
> 
> Action: QueueRemove
> Queue: 4152000400
> Interface: Local/4152000501 at from-internal/n
> 
> 
> CLI:
> ====
> > queue add member Local/4152000501 at from-internal/n to 4152000400 penalty 0 as "Edward Frank" state_interface Sip/SOFTJPS
> > queue remove member Local/4152000501 at from-internal/n from 4152000400
> 
> > queue add member Local/4152000501 at from-internal/n to 4152000400
> > queue remove member Local/4152000501 at from-internal/n from 4152000400
> 
> Dialplan:
> =========
> 
> [from-test]
> exten => 1234,1,AddQueueMember(4152000400,Local/4152000501 at from-internal/n,0,,Edward Davis,Sip/SOFTJPS)
> exten => 1234,n,RemoveQueueMember(4152000400,Local/4152000501 at from-internal/n)
> exten => 1234,n,Hangup()
> 
> exten => 5678,1,AddQueueMember(4152000400,Local/4152000501 at from-internal/n,0)
> exten => 5678,n,RemoveQueueMember(4152000400,Local/4152000501 at from-internal/n)
> exten => 5678,n,Hangup()
> 
> 
> Rinse and repeat with log_membername_as_agent = no
> Repeat with log_membername_as_agent omitted.
> 
> 
> Thanks,
> 
> jamuel
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20110714/90851963/attachment.htm>