Re: [asterisk-dev] [Code Review] 4437: dns: Define a core DNS API with examples.

[Joshua Colp]

> On Feb. 26, 2015, 1:53 a.m., Ashley Sanders wrote:
> > These are my findings regarding the first few sections of the document. On 
> > the technical-side, I have only made one small observation. All other 
> > observations are related to phrasing or a request for more information. At 
> > this time, I am still reviewing the API and examples sections.
> > 
> > 
> > As a general observation, I suggest stating somewhere in this document the 
> > RFCs we are supporting with the API. I have seen other DNS server 
> > implementations do something similar on their project's home and/or wiki 
> > pages.
> > 
> > 
> > Overview
> > 
> > "The Asterisk DNS API is designed as a thin wrapper over resolver 
> > implementations." By resolver implementations, plural, do you mean the DNS 
> > server will serve as a thin wrapper around implementations of a 
> > full-service resolver and a stub resolver?
> > 
> > So that your intentions are clearly conveyed from the start, I suggest that 
> > both occurrences of the phrase "some dns types" (last sentence in the 
> > overview) should be replaced with the concrete dns lookup/request/record 
> > types that this implementation will support.
> > 
> > 
> > Threading
> > 
> > Perhaps consider rephrasing this section to something like:
> > 
> > "The API provides a guarantee that it is safe to initiate and cancel 
> > queries from any thread. It does NOT, however, provide a guarantee that the 
> > ordering of queries shall be preserved. In other words, if you execute 
> > multiple queries individually, you will not necessarily receive callbacks 
> > in the order the queries were originally executed. This API only guarantees 
> > that an individual DNS result and records are safe to retrieve within the 
> > scope of the respective asynchronous callback."
> > 
> > Also, I think, if we receive a 'cancel' query, that should trump any other 
> > queries currently in the queue. Imho, it wouldn't make much sense to make a 
> > user wait for the queue to empty before executing the cancel (assuming the 
> > cancel query was last in line).
> > 
> > 
> > Query
> > 
> > I think we should provide more information here about the way we intend to 
> > implement the mechanics of queries under the hood: 
> > 
> > From wikipedia 
> > (http://en.wikipedia.org/wiki/Domain_Name_System#DNS_resolvers):
> > "A DNS query may be either a non-recursive query or a recursive query:
> > 
> > A non-recursive query is one in which the DNS server provides a record for 
> > a domain for which it is authoritative itself, or it provides a partial 
> > result without querying other servers.
> > A recursive query is one for which the DNS server will fully answer the 
> > query (or give an error) by querying other name servers as needed. DNS 
> > servers are not required to support recursive queries."
> > 
> > 
> > Result
> > As a consumer of the API, I would want to know specifically the kind(s) of 
> > information about the DNS query contained in the result. Also, does this 
> > return an error if it is impossible to get an answer for the query?
> > 
> > 
> > Record - Here is where I would go into more detail the types of records we 
> > support
> > 
> >

The RFCs depend on the resolver implementation itself. I'm also not sure about 
documenting record types. Resolver libraries generally don't care - they just 
provide the raw record back and it's up to you to interpret and parse them.  As 
for cancelling - there is no queue. If you cancel a query, it's cancelled then. 
How queries behave also depend on the resolver implementation itself (and its 
configuration). I'll extend the result with error information if I can.


- Joshua


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


On Feb. 23, 2015, 12:25 a.m., Joshua Colp wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4437/
> ---
> 
> (Updated Feb. 23, 2015, 12:25 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> A wiki page is present at:
> 
> https://wiki.asterisk.org/wiki/display/~jcolp/Asterisk+DNS+API
> 
> Which details a new DNS API for Asterisk. This exists as a thin wrapper over 
> other resolver libraries. The core part provides a common interface and some 
> additional features, such as NAPTR/SRV parsing and recurring lookups. 
> Examples are provided which cover the common use cases for the API.
> 
> Some stuff to think about:
> 
> 1. Does this encompass everything we think a low level API should?
> 2. Are there any higher level APIs that would be useful to have?
> 3. Is the usage intuitive and easy?
> 4. Are there other examples wh

Re: [asterisk-dev] [Code Review] 4437: dns: Define a core DNS API with examples.

[Ashley Sanders]

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


These are my findings regarding the first few sections of the document. On the 
technical-side, I have only made one small observation. All other observations 
are related to phrasing or a request for more information. At this time, I am 
still reviewing the API and examples sections.


As a general observation, I suggest stating somewhere in this document the RFCs 
we are supporting with the API. I have seen other DNS server implementations do 
something similar on their project's home and/or wiki pages.


Overview

"The Asterisk DNS API is designed as a thin wrapper over resolver 
implementations." By resolver implementations, plural, do you mean the DNS 
server will serve as a thin wrapper around implementations of a full-service 
resolver and a stub resolver?

So that your intentions are clearly conveyed from the start, I suggest that 
both occurrences of the phrase "some dns types" (last sentence in the overview) 
should be replaced with the concrete dns lookup/request/record types that this 
implementation will support.


Threading

Perhaps consider rephrasing this section to something like:

"The API provides a guarantee that it is safe to initiate and cancel queries 
from any thread. It does NOT, however, provide a guarantee that the ordering of 
queries shall be preserved. In other words, if you execute multiple queries 
individually, you will not necessarily receive callbacks in the order the 
queries were originally executed. This API only guarantees that an individual 
DNS result and records are safe to retrieve within the scope of the respective 
asynchronous callback."

Also, I think, if we receive a 'cancel' query, that should trump any other 
queries currently in the queue. Imho, it wouldn't make much sense to make a 
user wait for the queue to empty before executing the cancel (assuming the 
cancel query was last in line).


Query

I think we should provide more information here about the way we intend to 
implement the mechanics of queries under the hood: 

>From wikipedia (http://en.wikipedia.org/wiki/Domain_Name_System#DNS_resolvers):
"A DNS query may be either a non-recursive query or a recursive query:

A non-recursive query is one in which the DNS server provides a record for a 
domain for which it is authoritative itself, or it provides a partial result 
without querying other servers.
A recursive query is one for which the DNS server will fully answer the query 
(or give an error) by querying other name servers as needed. DNS servers are 
not required to support recursive queries."


Result
As a consumer of the API, I would want to know specifically the kind(s) of 
information about the DNS query contained in the result. Also, does this return 
an error if it is impossible to get an answer for the query?


Record - Here is where I would go into more detail the types of records we 
support



- Ashley Sanders


On Feb. 22, 2015, 6:25 p.m., Joshua Colp wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4437/
> ---
> 
> (Updated Feb. 22, 2015, 6:25 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> A wiki page is present at:
> 
> https://wiki.asterisk.org/wiki/display/~jcolp/Asterisk+DNS+API
> 
> Which details a new DNS API for Asterisk. This exists as a thin wrapper over 
> other resolver libraries. The core part provides a common interface and some 
> additional features, such as NAPTR/SRV parsing and recurring lookups. 
> Examples are provided which cover the common use cases for the API.
> 
> Some stuff to think about:
> 
> 1. Does this encompass everything we think a low level API should?
> 2. Are there any higher level APIs that would be useful to have?
> 3. Is the usage intuitive and easy?
> 4. Are there other examples which would help?
> 5. Do we want resolvers to be actual modules or keep them in-core?
> 6. Anything else you think of
> 
> Have at it!
> 
> 
> Diffs
> -
> 
> 
> Diff: https://reviewboard.asterisk.org/r/4437/diff/
> 
> 
> Testing
> ---
> 
> I've logically run through the API and examples to ensure they provide what 
> is needed for the future, to make them as easy as possible to use, and to 
> ensure higher level APIs can be created.
> 
> 
> Thanks,
> 
> Joshua Colp
> 
>

-- 
_
-- 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

Re: [asterisk-dev] [Code Review] 4379: Example configuration scenario - Super Awesome Company: Phase 1 - Patch 1

[rnewton]

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

(Updated Feb. 25, 2015, 5:48 p.m.)


Status
--

This change has been marked as submitted.


Review request for Asterisk Developers.


Changes
---

Committed in revision 432301


Repository: Asterisk


Description
---

One of things discussed at the last AstriDevCon was better documentation (for 
everything!), but in particular, we mentioned needing some example 
configurations that pertain to a real-world scenario. That is, as opposed to 
the current "sample" files which are sort of all over the place at this point.

This patch proposes a basic and minimal configuration of Asterisk to satisfy 
the requirements for the first phase of Super Awesome Company's implementation 
of Asterisk.

I will submit four separate patches for the first phase, so that we don't have 
to review the entire thing all at once. This review is for the first patch.

Who is Super Awesome Company? See 
https://wiki.asterisk.org/wiki/display/AST/Super+Awesome+Company

For the first patch, I am attempting to satisfy the below requirements. The 
patch does not include a new make target, as I believe Matt Jordan offered to 
handle that.

SAC requires:

* PJSIP connectivity for all employee desk phones.
* The ability for employees to call one another inside of the office.
* Voicemail boxes for each of the employees.

"Basic" configuration

We want SAC to have a clean system. That means:

* No 'autoload' in modules.conf. Explicitly load a basic configuration. If 
SAC doesn't need the module, don't load it.
* Every module loaded should have a configuration file that is appropriate 
for it. This includes all the 'core' things that need configuration.

pjsip.conf

* A PJSIP configuration for their desk phones. Assume every endpoint that 
is a phone has:
* A voicemail mailbox that they can subscribe to
* A hint for their device
* Note that the PJSIP configuration should adhere to best practices. 
That means MAC addresses for device names, etc.

extensions.conf

* A safe dialplan for intra-company communication. This should be templated 
out so that it is trivial to add additional devices (use pattern 
matching/pattern matching hints, etc.)
* Receiving a Busy/Unavailable should result in going to VoiceMail
* A user should be able to dial something and get to their VoiceMailMain 
without having to enter in their extension number 
* Note that mapping of MAC address endpoints to extension numbers should be 
done in some fashion that is easily extensible.

voicemail.conf

* Set up mailboxes for every person in SAC. Assign 'default' pins. Create 
reasonable basic settings.
* Do not set up e-mail or pager addresses.


REVIEW?

Please, if possible look at this from a few angles:

 * Use the configuration, configure a couple phones and call between them. 
Leave voicemails and retrieve them.
 * Have I created any security issues?
 * Is my dialplan easy to understand?
 * Could anything be done more efficiently without making it over-complicated?
 * Have I over-complicated anything?
 * Are there any critical settings I'm missing from any of the files?

A couple, more specific questions:

 * We have sample configs in /configs/samples; what directory do we want these 
configurations in? (I used /configs/examples for now, but I don't really like 
it)
 * We have the make target "make samples" for the current samples; what do we 
want for these new configs?


Diffs
-

  /branches/13/configs/examples/awesome/voicemail.conf PRE-CREATION 
  /branches/13/configs/examples/awesome/pjsip.conf PRE-CREATION 
  /branches/13/configs/examples/awesome/musiconhold.conf PRE-CREATION 
  /branches/13/configs/examples/awesome/modules.conf PRE-CREATION 
  /branches/13/configs/examples/awesome/logger.conf PRE-CREATION 
  /branches/13/configs/examples/awesome/indications.conf PRE-CREATION 
  /branches/13/configs/examples/awesome/extensions.conf PRE-CREATION 
  /branches/13/configs/examples/awesome/asterisk.conf PRE-CREATION 
  /branches/13/configs/examples/awesome/README PRE-CREATION 

Diff: https://reviewboard.asterisk.org/r/4379/diff/


Testing
---

Setup Asterisk with configuration, connected up three phones using the first 
three users. Made calls between them all, left voicemails and retrieved them 
with all users. Verified MWI working with all phones.


Thanks,

rnewton

-- 
_
-- 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

Re: [asterisk-dev] [Code Review] 4450: locatime.c file descriptor leak on kqueue(2) systems

[Ed Hynan]

> On Feb. 25, 2015, 6:15 p.m., Matt Jordan wrote:
> > tags/11.7.0/main/stdtime/localtime.c, lines 482-484
> > 
> >
> > I'm not sure why we are printing out to stderr here, but generally 
> > Asterisk does not do that unless (a) the logger couldn't be created and (b) 
> > things have gone horribly wrong. That doesn't seem like that would 
> > necessarily be the case here.

I agree, no need for a message there. Re. stderr, it's used several other 
places in original code, so it seemed preferred in this context.


> On Feb. 25, 2015, 6:15 p.m., Matt Jordan wrote:
> > tags/11.7.0/main/stdtime/localtime.c, lines 566-587
> > 
> >
> > This is not actually thread safe. You cannot guarantee that two 
> > threads, at the same time, won't attempt to test and allocate the structure 
> > and assign it to psx_sp.
> > 
> > What's more, the initialization of localtime happens very early in the 
> > Asterisk startup, and has some very interesting race conditions with 
> > forking/threading. I would definitely protect initialization of the psx_sp 
> > pointer with a lock.

Good eyes. I proceeded thinking calls to tzload() (which calls aadd_notify()) 
were guarded by AST_LIST_LOCK(&zonelist) but another look now shows a place 
where tzload()(and tzparse() and gmtload()) are called between an unlock and 
another lock (ast_tzset() line 1451 unpatched).  Yet other places make the 
calls while the lock is held.

This is an extra complication.


> On Feb. 25, 2015, 6:15 p.m., Matt Jordan wrote:
> > tags/11.7.0/main/stdtime/localtime.c, lines 293-297
> > 
> >
> > We typically don't leave unused code lying around. I'd go ahead and 
> > remove this section if it isn't useful.

OK. That's only there because a look at the manpage made me wonder about it; 
hope was that persons more familiar with Linux inotify would clear it up.

I'll remove it for next patch.


- Ed


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


On Feb. 25, 2015, 4:37 p.m., Ed Hynan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4450/
> ---
> 
> (Updated Feb. 25, 2015, 4:37 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24739
> https://issues.asterisk.org/jira/browse/ASTERISK-24739
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> File descriptor leak on kqueue(2) systems (found on OpenBSD 5.5) on 
> main/stdtime/localtime.c -- TZ data files change watch code within the time 
> functions. (Issue at https://issues.asterisk.org/jira/browse/ASTERISK-24739.)
> 
> 
> Diffs
> -
> 
>   tags/11.7.0/main/stdtime/localtime.c 432235 
> 
> Diff: https://reviewboard.asterisk.org/r/4450/diff/
> 
> 
> Testing
> ---
> 
> Patch initially developed against OpenBSD 5.5 ports package sources, and 
> Asterisk in use. Subsequently developed test program that can run original or 
> patched 11.7.0 code.
> 
> 
> File Attachments
> 
> 
> localtime.c (11.7.0) test program
>   
> https://reviewboard.asterisk.org/media/uploaded/files/2015/02/25/f7a19f2a-4a8a-4aff-b7d6-44a4146c358c__patchtest.tgz
> 
> 
> Thanks,
> 
> Ed Hynan
> 
>

-- 
_
-- 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

Re: [asterisk-dev] [Code Review] 4420: testsuite: Add nominal and off-nominal SRTP negotiation tests for key lifetime/MKI

[Matt Jordan]

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

(Updated Feb. 25, 2015, 3:49 p.m.)


Status
--

This change has been marked as submitted.


Review request for Asterisk Developers and Olle E Johansson.


Changes
---

Committed in revision 6459


Bugs: ASTERISK-17721, ASTERISK-17899 and ASTERISK-22748
https://issues.asterisk.org/jira/browse/ASTERISK-17721
https://issues.asterisk.org/jira/browse/ASTERISK-17899
https://issues.asterisk.org/jira/browse/ASTERISK-22748


Repository: testsuite


Description
---

This patch adds nominal and off-nominal tests for SDP negotiation of SDES-SRTP 
crypto attributes, for both chan_sip and chan_pjsip.

Specifically:

* Moved the tests/channels/SIP/sip_srtp test to 
tests/channels/SIP/sip_srtp/srtp_call. This change was done merely to allow for 
additional SRTP tests for chan_sip, and is not part of this review. Note that 
due to a quirk of Review Board, the moved files don't show up in the review.

* Added tests/channels/SIP/sip_srtp/srtp_sdp_offer_answer. This covers both 
nominal and off-nominal SDP offers with SDES-SRTP for chan_sip. Note that the 
scenarios use injection files to vary the crypto attributes.

* Updated tests/channels/PJSIP/srtp_negotiation.
  - All but two off-nominal scenarios were moved into a single off nominal 
scenario, decline.xml. This uses the same injection file for off nominal 
parameter testing as the chan_sip variant.
  - Updated accept_nominal.xml to use an injection file to test multiple 
nominal values.
  - Updated accept_multiple_attrib_first_bad.xml to not use a crypto attribute 
with lifetime as a 'bad' attribute.


Diffs
-

  /asterisk/trunk/tests/channels/pjsip/srtp_negotiation/test-config.yaml 6415 
  
/asterisk/trunk/tests/channels/pjsip/srtp_negotiation/sipp/decline_with_lifetime.xml
 6415 
  
/asterisk/trunk/tests/channels/pjsip/srtp_negotiation/sipp/decline_unknown_suite.xml
 6415 
  /asterisk/trunk/tests/channels/pjsip/srtp_negotiation/sipp/decline_no_tag.xml 
6415 
  
/asterisk/trunk/tests/channels/pjsip/srtp_negotiation/sipp/decline_no_suite.xml 
6415 
  /asterisk/trunk/tests/channels/pjsip/srtp_negotiation/sipp/decline_no_key.xml 
6415 
  /asterisk/trunk/tests/channels/pjsip/srtp_negotiation/sipp/decline.xml 
PRE-CREATION 
  /asterisk/trunk/tests/channels/pjsip/srtp_negotiation/sipp/decline.csv 
PRE-CREATION 
  /asterisk/trunk/tests/channels/pjsip/srtp_negotiation/sipp/accept_nominal.xml 
6415 
  
/asterisk/trunk/tests/channels/pjsip/srtp_negotiation/sipp/accept_multiple_attrib_first_bad.xml
 6415 
  /asterisk/trunk/tests/channels/pjsip/srtp_negotiation/sipp/accept.csv 
PRE-CREATION 
  /asterisk/trunk/tests/channels/SIP/tests.yaml 6415 
  /asterisk/trunk/tests/channels/SIP/sip_srtp/test-config.yaml 6415 
  
/asterisk/trunk/tests/channels/SIP/sip_srtp/srtp_sdp_offer_answer/test-config.yaml
 PRE-CREATION 
  
/asterisk/trunk/tests/channels/SIP/sip_srtp/srtp_sdp_offer_answer/sipp/srtp-invite-single-crypto.xml
 PRE-CREATION 
  
/asterisk/trunk/tests/channels/SIP/sip_srtp/srtp_sdp_offer_answer/sipp/srtp-invite-off-nominal-crypto.xml
 PRE-CREATION 
  
/asterisk/trunk/tests/channels/SIP/sip_srtp/srtp_sdp_offer_answer/sipp/srtp-invite-double-crypto.xml
 PRE-CREATION 
  
/asterisk/trunk/tests/channels/SIP/sip_srtp/srtp_sdp_offer_answer/sipp/inject_attrib_single_off_nominal.csv
 PRE-CREATION 
  
/asterisk/trunk/tests/channels/SIP/sip_srtp/srtp_sdp_offer_answer/sipp/inject_attrib_single_nominal.csv
 PRE-CREATION 
  
/asterisk/trunk/tests/channels/SIP/sip_srtp/srtp_sdp_offer_answer/sipp/inject_attrib_double_nominal.csv
 PRE-CREATION 
  
/asterisk/trunk/tests/channels/SIP/sip_srtp/srtp_sdp_offer_answer/configs/ast1/sip.conf
 PRE-CREATION 
  
/asterisk/trunk/tests/channels/SIP/sip_srtp/srtp_sdp_offer_answer/configs/ast1/extensions.conf
 PRE-CREATION 
  /asterisk/trunk/tests/channels/SIP/sip_srtp/run-test 6415 
  /asterisk/trunk/tests/channels/SIP/sip_srtp/configs/ast2/sip.conf 6415 
  /asterisk/trunk/tests/channels/SIP/sip_srtp/configs/ast2/extensions.conf 6415 
  /asterisk/trunk/tests/channels/SIP/sip_srtp/configs/ast1/sip.conf 6415 
  /asterisk/trunk/tests/channels/SIP/sip_srtp/configs/ast1/extensions.conf 6415 

Diff: https://reviewboard.asterisk.org/r/4420/diff/


Testing
---


Thanks,

Matt Jordan

-- 
_
-- 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

Re: [asterisk-dev] [Code Review] 4418: SDES-SRTP: Handle SRTP keys negotiated with key lifetime/MKI (oej branch lingon-srtp-key-lifetime-1.8) - Asterisk 13

[Matt Jordan]

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

(Updated Feb. 25, 2015, 9:13 p.m.)


Status
--

This change has been marked as submitted.


Review request for Asterisk Developers and Olle E Johansson.


Changes
---

Committed in revision 432239


Bugs: ASTERISK-17721, ASTERISK-17899 and ASTERISK-22748
https://issues.asterisk.org/jira/browse/ASTERISK-17721
https://issues.asterisk.org/jira/browse/ASTERISK-17899
https://issues.asterisk.org/jira/browse/ASTERISK-22748


Repository: Asterisk


Description
---

Note that this patch is a forward port of oej's lingon-srtp-key-lifetime-1.8 
branch, with a few small modifications to reduce block indentation and a few 
improvements made to surrounding existing code.

This patch is for Asterisk 13, and is essentially identical to 
https://reviewboard.asterisk.org/r/4419 - save for a few small modifications 
for the public API written when the code was ported into the core. As such, 
this patch should only be reviewed for those changes that are not in r4419.


Diffs
-

  /branches/13/main/sdp_srtp.c 431750 

Diff: https://reviewboard.asterisk.org/r/4418/diff/


Testing
---

Tests were added for chan_sip and updated for chan_pjsip - see 
https://reviewboard.asterisk.org/r/4420 . This includes both nominal and 
off-nominal offers negotiating SDES-SRTP.


Thanks,

Matt Jordan

-- 
_
-- 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

Re: [asterisk-dev] [Code Review] 4419: SDES-SRTP: Handle SRTP keys negotiated with key lifetime/MKI (oej branch lingon-srtp-key-lifetime-1.8) - Asterisk 11

[Matt Jordan]

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

(Updated Feb. 25, 2015, 3:13 p.m.)


Status
--

This change has been marked as submitted.


Review request for Asterisk Developers and Olle E Johansson.


Changes
---

Committed in revision 432239


Bugs: ASTERISK-17721, ASTERISK-17899 and ASTERISK-22748
https://issues.asterisk.org/jira/browse/ASTERISK-17721
https://issues.asterisk.org/jira/browse/ASTERISK-17899
https://issues.asterisk.org/jira/browse/ASTERISK-22748


Repository: Asterisk


Description
---

Note that this patch is a forward port of oej's lingon-srtp-key-lifetime-1.8 
branch, with a few small modifications to reduce block indentation and a few 
improvements made to surrounding existing code.

To quote Olle from ASTERISK-17721:

{quote}
The Lingon branch now handle lifetime and MKI parameters.

We only accept lifetimes up to max for the crypto and higher than 10 hours for 
packetization of 20 ms (50 pps).

We only handle MKI with index 1.

We do not really bother with counting packets and reinviting at end of 
lifetime, so the min of 10 hours kind of takes care of most calls. If there are 
longer ones, we rely on the other side for re-invites.

It's still not perfect, but I personally think this is an improvement. A 
configuration option for minimum lifetime accepted could be added.
{quote}

I personally don't think the minimum lifetime option is needed, as in practice, 
every instance of an SDP offer for SDES-SRTP with lifetime I've seen offers a 
lifetime of 2^32 - but it could be added in the future if necessary.

Note that since the sdp_crypto code was moved to the core in 12+, a separate 
review (r4418) has been made for the Asterisk 13 variant. It is essentially 
identical to this review.


Diffs
-

  /branches/11/channels/sip/sdp_crypto.c 432011 

Diff: https://reviewboard.asterisk.org/r/4419/diff/


Testing
---

Tests were added for chan_sip and updated for chan_pjsip - see 
https://reviewboard.asterisk.org/r/4420 . This includes both nominal and 
off-nominal offers negotiating SDES-SRTP.


Thanks,

Matt Jordan

-- 
_
-- 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

Re: [asterisk-dev] [Code Review] 4431: Increase WebSocket frame size and improve large read handling

[David Lee]

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

(Updated Feb. 25, 2015, 2:46 p.m.)


Status
--

This change has been marked as submitted.


Review request for Asterisk Developers.


Changes
---

Committed in revision 432236


Repository: Asterisk


Description
---

Some WebSocket applications, like [chan_respoke][], require a larger
frame size than the default 8k; this patch bumps the default to 16k.
This patch also fixes some problems exacerbated by large frames.

The sanity counter was decremented on every fread attempt in
ws_safe_read(), regardless of whether data was read from the socket or
not. For large frames, this could result in loss of sanity prior to
reading the entire frame. (16k frame / 1448 bytes per segment = 12
segments).

This patch changes the sanity counter so that it only decrements when
fread() doesn't read any bytes. This more closely matches the original
intention of ws_safe_read(), given that the error message is
"Websocket seems unresponsive".

This patch also properly logs EOF conditions, so disconnects are no
longer confused with unresponsive connections.

 [chan_respoke]: https://github.com/respoke/chan_respoke


Diffs
-

  /branches/11/res/res_http_websocket.c 431915 

Diff: https://reviewboard.asterisk.org/r/4431/diff/


Testing
---

Ran a Node app that continuously send large WebSocket frame to Asterisk.

https://gist.github.com/leedm777/ba6d86468d7646073286

Without the patch, Asterisk fails in less than 10 frames. With the patch, it 
runs like a boss.


Thanks,

David Lee

-- 
_
-- 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

Re: [asterisk-dev] [Code Review] 4420: testsuite: Add nominal and off-nominal SRTP negotiation tests for key lifetime/MKI

[Joshua Colp]

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

Ship it!


Ship It!

- Joshua Colp


On Feb. 20, 2015, 2:59 a.m., Matt Jordan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4420/
> ---
> 
> (Updated Feb. 20, 2015, 2:59 a.m.)
> 
> 
> Review request for Asterisk Developers and Olle E Johansson.
> 
> 
> Bugs: ASTERISK-17721, ASTERISK-17899 and ASTERISK-22748
> https://issues.asterisk.org/jira/browse/ASTERISK-17721
> https://issues.asterisk.org/jira/browse/ASTERISK-17899
> https://issues.asterisk.org/jira/browse/ASTERISK-22748
> 
> 
> Repository: testsuite
> 
> 
> Description
> ---
> 
> This patch adds nominal and off-nominal tests for SDP negotiation of 
> SDES-SRTP crypto attributes, for both chan_sip and chan_pjsip.
> 
> Specifically:
> 
> * Moved the tests/channels/SIP/sip_srtp test to 
> tests/channels/SIP/sip_srtp/srtp_call. This change was done merely to allow 
> for additional SRTP tests for chan_sip, and is not part of this review. Note 
> that due to a quirk of Review Board, the moved files don't show up in the 
> review.
> 
> * Added tests/channels/SIP/sip_srtp/srtp_sdp_offer_answer. This covers both 
> nominal and off-nominal SDP offers with SDES-SRTP for chan_sip. Note that the 
> scenarios use injection files to vary the crypto attributes.
> 
> * Updated tests/channels/PJSIP/srtp_negotiation.
>   - All but two off-nominal scenarios were moved into a single off nominal 
> scenario, decline.xml. This uses the same injection file for off nominal 
> parameter testing as the chan_sip variant.
>   - Updated accept_nominal.xml to use an injection file to test multiple 
> nominal values.
>   - Updated accept_multiple_attrib_first_bad.xml to not use a crypto 
> attribute with lifetime as a 'bad' attribute.
> 
> 
> Diffs
> -
> 
>   /asterisk/trunk/tests/channels/pjsip/srtp_negotiation/test-config.yaml 6415 
>   
> /asterisk/trunk/tests/channels/pjsip/srtp_negotiation/sipp/decline_with_lifetime.xml
>  6415 
>   
> /asterisk/trunk/tests/channels/pjsip/srtp_negotiation/sipp/decline_unknown_suite.xml
>  6415 
>   
> /asterisk/trunk/tests/channels/pjsip/srtp_negotiation/sipp/decline_no_tag.xml 
> 6415 
>   
> /asterisk/trunk/tests/channels/pjsip/srtp_negotiation/sipp/decline_no_suite.xml
>  6415 
>   
> /asterisk/trunk/tests/channels/pjsip/srtp_negotiation/sipp/decline_no_key.xml 
> 6415 
>   /asterisk/trunk/tests/channels/pjsip/srtp_negotiation/sipp/decline.xml 
> PRE-CREATION 
>   /asterisk/trunk/tests/channels/pjsip/srtp_negotiation/sipp/decline.csv 
> PRE-CREATION 
>   
> /asterisk/trunk/tests/channels/pjsip/srtp_negotiation/sipp/accept_nominal.xml 
> 6415 
>   
> /asterisk/trunk/tests/channels/pjsip/srtp_negotiation/sipp/accept_multiple_attrib_first_bad.xml
>  6415 
>   /asterisk/trunk/tests/channels/pjsip/srtp_negotiation/sipp/accept.csv 
> PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/tests.yaml 6415 
>   /asterisk/trunk/tests/channels/SIP/sip_srtp/test-config.yaml 6415 
>   
> /asterisk/trunk/tests/channels/SIP/sip_srtp/srtp_sdp_offer_answer/test-config.yaml
>  PRE-CREATION 
>   
> /asterisk/trunk/tests/channels/SIP/sip_srtp/srtp_sdp_offer_answer/sipp/srtp-invite-single-crypto.xml
>  PRE-CREATION 
>   
> /asterisk/trunk/tests/channels/SIP/sip_srtp/srtp_sdp_offer_answer/sipp/srtp-invite-off-nominal-crypto.xml
>  PRE-CREATION 
>   
> /asterisk/trunk/tests/channels/SIP/sip_srtp/srtp_sdp_offer_answer/sipp/srtp-invite-double-crypto.xml
>  PRE-CREATION 
>   
> /asterisk/trunk/tests/channels/SIP/sip_srtp/srtp_sdp_offer_answer/sipp/inject_attrib_single_off_nominal.csv
>  PRE-CREATION 
>   
> /asterisk/trunk/tests/channels/SIP/sip_srtp/srtp_sdp_offer_answer/sipp/inject_attrib_single_nominal.csv
>  PRE-CREATION 
>   
> /asterisk/trunk/tests/channels/SIP/sip_srtp/srtp_sdp_offer_answer/sipp/inject_attrib_double_nominal.csv
>  PRE-CREATION 
>   
> /asterisk/trunk/tests/channels/SIP/sip_srtp/srtp_sdp_offer_answer/configs/ast1/sip.conf
>  PRE-CREATION 
>   
> /asterisk/trunk/tests/channels/SIP/sip_srtp/srtp_sdp_offer_answer/configs/ast1/extensions.conf
>  PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_srtp/run-test 6415 
>   /asterisk/trunk/tests/channels/SIP/sip_srtp/configs/ast2/sip.conf 6415 
>   /asterisk/trunk/tests/channels/SIP/sip_srtp/configs/ast2/extensions.conf 
> 6415 
>   /asterisk/trunk/tests/channels/SIP/sip_srtp/configs/ast1/sip.conf 6415 
>   /asterisk/trunk/tests/channels/SIP/sip_srtp/configs/ast1/extensions.conf 
> 6415 
> 
> Diff: https://reviewboard.asterisk.org/r/4420/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Matt Jordan
> 
>

-- 

Re: [asterisk-dev] [Code Review] 4450: locatime.c file descriptor leak on kqueue(2) systems

[Matt Jordan]

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



tags/11.7.0/main/stdtime/localtime.c


Having the various blocks of SP_INIT_EX/SP_FREE_EX defined around the code 
is actually a bit confusing, particularly since the structure they 
initialize/destroy already has various #defines for the various system options 
it has to support. How about having this defined immediately after struct 
state, and handling the "flavors" there:

struct state {
...
};

#if defined(HAVE_INOTIFY)
#define SP_INIT_EX ...
#define SP_FREE_EX ...
#elif defined(HAVE_KQUEUE)
#define SP_INIT_EX ...
#define SP_FREE_EX ...
#else
#define SP_INIT_EX ...
#define SP_FREE_EX ...
#endif

That way it's clear why this is getting defined multiple times, and the 
declaration of the macro immediately following the definition of the structure 
makes it easy to see why the various variants exist.



tags/11.7.0/main/stdtime/localtime.c


We typically don't leave unused code lying around. I'd go ahead and remove 
this section if it isn't useful.



tags/11.7.0/main/stdtime/localtime.c


Using a global static struct without locking is not thread safe. More on 
this below.



tags/11.7.0/main/stdtime/localtime.c


If you move kqueue_daemon_freestate(struct state *sp) above kqueue_daemon, 
then you won't need this forward declaration.



tags/11.7.0/main/stdtime/localtime.c


I'm not sure why we are printing out to stderr here, but generally Asterisk 
does not do that unless (a) the logger couldn't be created and (b) things have 
gone horribly wrong. That doesn't seem like that would necessarily be the case 
here.



tags/11.7.0/main/stdtime/localtime.c


This is not actually thread safe. You cannot guarantee that two threads, at 
the same time, won't attempt to test and allocate the structure and assign it 
to psx_sp.

What's more, the initialization of localtime happens very early in the 
Asterisk startup, and has some very interesting race conditions with 
forking/threading. I would definitely protect initialization of the psx_sp 
pointer with a lock.



tags/11.7.0/main/stdtime/localtime.c


Generally, we tend to prefer allocations to be written as:

p = ast_calloc(1, sizeof(*p));
if (!p) {
/* Handle memory error */
}

It generally prevents paranthesis errors, and newlines are relatively 
inexpensive :-)



tags/11.7.0/main/stdtime/localtime.c


Since ast_free is NULL tolerant, I would go ahead and make SP_FREE_EX NULL 
tolerant as well. That makes this:

static void sstate_free(struct state *p)
{
SP_FREE_EX(p);
ast_free(p);
}



tags/11.7.0/main/stdtime/localtime.c


This shouldn't be necessary. Called in a loop, AST_LIST_REMOVE_HEAD will:

(1) Remove items until (cur = head->first) == NULL. That implies 
head->first is NULL when complete.
(2) Will set head->last = NULL when the (cur == head->last), which should 
happen on the last loop iteration.

Since AST_LIST_HEAD_INIT_NOLOCK simply sets both of those to NULL, it is 
redundant with the final loop state.


- Matt Jordan


On Feb. 25, 2015, 10:37 a.m., Ed Hynan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4450/
> ---
> 
> (Updated Feb. 25, 2015, 10:37 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24739
> https://issues.asterisk.org/jira/browse/ASTERISK-24739
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> File descriptor leak on kqueue(2) systems (found on OpenBSD 5.5) on 
> main/stdtime/localtime.c -- TZ data files change watch code within the time 
> functions. (Issue at https://issues.asterisk.org/jira/browse/ASTERISK-24739.)
> 
> 
> Diffs
> -
> 
>   tags/11.7.0/main/stdtime/localtime.c 432235 
> 
> Diff: https://reviewboard.asterisk.org/r/4450/diff/
> 
> 
> Testing
> ---
> 
> Patch initially developed against OpenBSD 5.5 ports package sources, and 
> Asterisk in use. Subsequently developed test program that can run original or 
> patched 11.7.0 code.
> 
> 
> File Attachments
> 

Re: [asterisk-dev] [Code Review] 4451: testsuite: Add DNS server pluggable module.

[Joshua Colp]

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

(Updated Feb. 25, 2015, 5:57 p.m.)


Review request for Asterisk Developers.


Repository: testsuite


Description
---

This change adds a pluggable module to the testsuite which runs a DNS server 
using twisted-names. This is an authoritative DNS server that responds with 
records which are specified in a Python style zone file or a BIND style zone 
file. The DNS server lives for the lifetime of the test. This is useful since 
it removes any reliance on an external DNS server and reduces the work required 
to test DNS.


Diffs (updated)
-

  /asterisk/trunk/sample-yaml/dns-server-config.yaml PRE-CREATION 
  /asterisk/trunk/lib/python/asterisk/dns_server.py PRE-CREATION 

Diff: https://reviewboard.asterisk.org/r/4451/diff/


Testing
---

Hacked up a quick test which used the pluggable module to start a DNS server 
for a domain. Used dig to contact the DNS server and confirm records.


Thanks,

Joshua Colp

-- 
_
-- 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

Re: [asterisk-dev] [Code Review] 4447: ARI: Fix crash if integer values used in JSON payload 'variables' object.

[Matt Jordan]

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

Ship it!


Ship It!

- Matt Jordan


On Feb. 25, 2015, 10:21 a.m., rmudgett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4447/
> ---
> 
> (Updated Feb. 25, 2015, 10:21 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24751
> https://issues.asterisk.org/jira/browse/ASTERISK-24751
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> Sending the following ARI commands caused Asterisk to crash if the JSON
> body 'variables' object passes values of types other than strings.
> 
> POST /ari/channels
> POST /ari/channels/{channelid}
> PUT /ari/endpoints/sendMessage
> PUT /ari/endpoints/{tech}/{resource}/sendMessage
> 
> * Eliminated RAII_VAR usage in ast_ari_channels_originate_with_id(),
> ast_ari_channels_originate(), ast_ari_endpoints_send_message(), and
> ast_ari_endpoints_send_message_to_endpoint().
> 
> 
> Diffs
> -
> 
>   /branches/13/rest-api/api-docs/endpoints.json 432235 
>   /branches/13/res/res_ari_endpoints.c 432235 
>   /branches/13/res/ari/resource_endpoints.c 432235 
>   /branches/13/res/ari/resource_channels.c 432235 
>   /branches/13/main/json.c 432235 
>   /branches/13/include/asterisk/json.h 432235 
> 
> Diff: https://reviewboard.asterisk.org/r/4447/diff/
> 
> 
> Testing
> ---
> 
> The four commands no longer crash and now report 400 Bad Request with a
> message that the 'variables' object only accepts string values when I
> pass an integer value.
> 
> 
> Thanks,
> 
> rmudgett
> 
>

-- 
_
-- 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

Re: [asterisk-dev] [Code Review] 4451: testsuite: Add DNS server pluggable module.

[Matt Jordan]

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

Ship it!


That is surprisingly small. Yay.


/asterisk/trunk/lib/python/asterisk/dns_server.py


Go ahead and add a sample configuration YAML file in the sample-yaml 
directory.

Configuring BIND zone files are beyond the scope of this :-)


- Matt Jordan


On Feb. 25, 2015, 11:05 a.m., Joshua Colp wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4451/
> ---
> 
> (Updated Feb. 25, 2015, 11:05 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: testsuite
> 
> 
> Description
> ---
> 
> This change adds a pluggable module to the testsuite which runs a DNS server 
> using twisted-names. This is an authoritative DNS server that responds with 
> records which are specified in a Python style zone file or a BIND style zone 
> file. The DNS server lives for the lifetime of the test. This is useful since 
> it removes any reliance on an external DNS server and reduces the work 
> required to test DNS.
> 
> 
> Diffs
> -
> 
>   /asterisk/trunk/lib/python/asterisk/dns_server.py PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/4451/diff/
> 
> 
> Testing
> ---
> 
> Hacked up a quick test which used the pluggable module to start a DNS server 
> for a domain. Used dig to contact the DNS server and confirm records.
> 
> 
> Thanks,
> 
> Joshua Colp
> 
>

-- 
_
-- 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

[asterisk-dev] [Code Review] 4451: testsuite: Add DNS server pluggable module.

[Joshua Colp]

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

Review request for Asterisk Developers.


Repository: testsuite


Description
---

This change adds a pluggable module to the testsuite which runs a DNS server 
using twisted-names. This is an authoritative DNS server that responds with 
records which are specified in a Python style zone file or a BIND style zone 
file. The DNS server lives for the lifetime of the test. This is useful since 
it removes any reliance on an external DNS server and reduces the work required 
to test DNS.


Diffs
-

  /asterisk/trunk/lib/python/asterisk/dns_server.py PRE-CREATION 

Diff: https://reviewboard.asterisk.org/r/4451/diff/


Testing
---

Hacked up a quick test which used the pluggable module to start a DNS server 
for a domain. Used dig to contact the DNS server and confirm records.


Thanks,

Joshua Colp

-- 
_
-- 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

[asterisk-dev] [Code Review] 4450: locatime.c file descriptor leak on kqueue(2) systems

[Ed Hynan]

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

Review request for Asterisk Developers.


Bugs: ASTERISK-24739
https://issues.asterisk.org/jira/browse/ASTERISK-24739


Repository: Asterisk


Description
---

File descriptor leak on kqueue(2) systems (found on OpenBSD 5.5) on 
main/stdtime/localtime.c -- TZ data files change watch code within the time 
functions. (Issue at https://issues.asterisk.org/jira/browse/ASTERISK-24739.)


Diffs
-

  tags/11.7.0/main/stdtime/localtime.c 432235 

Diff: https://reviewboard.asterisk.org/r/4450/diff/


Testing
---

Patch initially developed against OpenBSD 5.5 ports package sources, and 
Asterisk in use. Subsequently developed test program that can run original or 
patched 11.7.0 code.


File Attachments


localtime.c (11.7.0) test program
  
https://reviewboard.asterisk.org/media/uploaded/files/2015/02/25/f7a19f2a-4a8a-4aff-b7d6-44a4146c358c__patchtest.tgz


Thanks,

Ed Hynan

-- 
_
-- 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

Re: [asterisk-dev] [Code Review] 4447: ARI: Fix crash if integer values used in JSON payload 'variables' object.

[rmudgett]

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

(Updated Feb. 25, 2015, 10:21 a.m.)


Review request for Asterisk Developers.


Changes
---

Moar Matt feedback.


Bugs: ASTERISK-24751
https://issues.asterisk.org/jira/browse/ASTERISK-24751


Repository: Asterisk


Description
---

Sending the following ARI commands caused Asterisk to crash if the JSON
body 'variables' object passes values of types other than strings.

POST /ari/channels
POST /ari/channels/{channelid}
PUT /ari/endpoints/sendMessage
PUT /ari/endpoints/{tech}/{resource}/sendMessage

* Eliminated RAII_VAR usage in ast_ari_channels_originate_with_id(),
ast_ari_channels_originate(), ast_ari_endpoints_send_message(), and
ast_ari_endpoints_send_message_to_endpoint().


Diffs (updated)
-

  /branches/13/rest-api/api-docs/endpoints.json 432235 
  /branches/13/res/res_ari_endpoints.c 432235 
  /branches/13/res/ari/resource_endpoints.c 432235 
  /branches/13/res/ari/resource_channels.c 432235 
  /branches/13/main/json.c 432235 
  /branches/13/include/asterisk/json.h 432235 

Diff: https://reviewboard.asterisk.org/r/4447/diff/


Testing
---

The four commands no longer crash and now report 400 Bad Request with a
message that the 'variables' object only accepts string values when I
pass an integer value.


Thanks,

rmudgett

-- 
_
-- 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

Re: [asterisk-dev] [Code Review] 4447: ARI: Fix crash if integer values used in JSON payload 'variables' object.

[Matt Jordan]

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



/branches/13/main/json.c


I think we should allow for empty value strings. Consider the following 
JSON:

{
"key1": "value",
"key2": ""
}

This patch would cause key2 to be skipped, resulting in whatever value was 
there to be preserved.

I think instead we would just want:

value = ast_json_string_get(json_value);
if (!value) {
   continue;
}

That is, if we managed to get a NULL pointer back from ast_json_string_get, 
that's a "skip" - but getting a valid string that is immediately terminated 
should be allowed as a value for a key.


- Matt Jordan


On Feb. 25, 2015, 9:27 a.m., rmudgett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4447/
> ---
> 
> (Updated Feb. 25, 2015, 9:27 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24751
> https://issues.asterisk.org/jira/browse/ASTERISK-24751
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> Sending the following ARI commands caused Asterisk to crash if the JSON
> body 'variables' object passes values of types other than strings.
> 
> POST /ari/channels
> POST /ari/channels/{channelid}
> PUT /ari/endpoints/sendMessage
> PUT /ari/endpoints/{tech}/{resource}/sendMessage
> 
> * Eliminated RAII_VAR usage in ast_ari_channels_originate_with_id(),
> ast_ari_channels_originate(), ast_ari_endpoints_send_message(), and
> ast_ari_endpoints_send_message_to_endpoint().
> 
> 
> Diffs
> -
> 
>   /branches/13/rest-api/api-docs/endpoints.json 432235 
>   /branches/13/res/res_ari_endpoints.c 432235 
>   /branches/13/res/ari/resource_endpoints.c 432235 
>   /branches/13/res/ari/resource_channels.c 432235 
>   /branches/13/main/json.c 432235 
>   /branches/13/include/asterisk/json.h 432235 
> 
> Diff: https://reviewboard.asterisk.org/r/4447/diff/
> 
> 
> Testing
> ---
> 
> The four commands no longer crash and now report 400 Bad Request with a
> message that the 'variables' object only accepts string values when I
> pass an integer value.
> 
> 
> Thanks,
> 
> rmudgett
> 
>

-- 
_
-- 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

Re: [asterisk-dev] [Code Review] 4445: app_chanspy, channel: fix frame leaks

[rmudgett]

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

Ship it!


Ship It!

- rmudgett


On Feb. 24, 2015, 5:05 p.m., Kevin Harwell wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4445/
> ---
> 
> (Updated Feb. 24, 2015, 5:05 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24828
> https://issues.asterisk.org/jira/browse/ASTERISK-24828
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> During testing it was noticed that Asterisk was leaking frames. After doing 
> some digging a couple spots where a frame leak could occur were found. This 
> patch plugs those leaks.
> 
> 
> Diffs
> -
> 
>   branches/11/main/channel.c 432173 
>   branches/11/apps/app_chanspy.c 432173 
> 
> Diff: https://reviewboard.asterisk.org/r/4445/diff/
> 
> 
> Testing
> ---
> 
> After applying the patch testing was done again to make sure the leaks no 
> longer occurred.
> 
> 
> Thanks,
> 
> Kevin Harwell
> 
>

-- 
_
-- 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

Re: [asterisk-dev] [Code Review] 4447: ARI: Fix crash if integer values used in JSON payload 'variables' object.

[rmudgett]

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

(Updated Feb. 25, 2015, 9:27 a.m.)


Review request for Asterisk Developers.


Changes
---

Address Matt's feedback.


Bugs: ASTERISK-24751
https://issues.asterisk.org/jira/browse/ASTERISK-24751


Repository: Asterisk


Description
---

Sending the following ARI commands caused Asterisk to crash if the JSON
body 'variables' object passes values of types other than strings.

POST /ari/channels
POST /ari/channels/{channelid}
PUT /ari/endpoints/sendMessage
PUT /ari/endpoints/{tech}/{resource}/sendMessage

* Eliminated RAII_VAR usage in ast_ari_channels_originate_with_id(),
ast_ari_channels_originate(), ast_ari_endpoints_send_message(), and
ast_ari_endpoints_send_message_to_endpoint().


Diffs (updated)
-

  /branches/13/rest-api/api-docs/endpoints.json 432235 
  /branches/13/res/res_ari_endpoints.c 432235 
  /branches/13/res/ari/resource_endpoints.c 432235 
  /branches/13/res/ari/resource_channels.c 432235 
  /branches/13/main/json.c 432235 
  /branches/13/include/asterisk/json.h 432235 

Diff: https://reviewboard.asterisk.org/r/4447/diff/


Testing
---

The four commands no longer crash and now report 400 Bad Request with a
message that the 'variables' object only accepts string values when I
pass an integer value.


Thanks,

rmudgett

-- 
_
-- 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

Re: [asterisk-dev] [Code Review] 4437: dns: Define a core DNS API with examples.

[Olle E. Johansson]

On 25 Feb 2015, at 15:25, Joshua Colp  wrote:

> James Cloos wrote:
>>> "J" == Joshua Colp  writes:
>> 
>> J>  2. Are there any higher level APIs that would be useful to have?
>> 
>> In addition to the dns_naptr.h and dns_srv.h apis, there should be a
>> similar dns_tlsa.h api.
>> 
>> Even if tlsa is not yet widespread, Asterisk's dns code should offer
>> quality support for it.  And Asterisk should use them.
> 
> I've added this as a comment on the review so it's all nicely together if 
> someone looks at things.

+1

I did mention this earlier...

/O
-- 
_
-- 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

Re: [asterisk-dev] [Code Review] 4437: dns: Define a core DNS API with examples.

[Joshua Colp]

> On Feb. 24, 2015, 11:48 p.m., Mark Michelson wrote:
> > First off, good page! It's pretty easy to follow the expected flow for DNS 
> > queries based on the API you've provided. From a low-level, there isn't 
> > much that's missing: you have ways of performing synchronous and 
> > asynchronous DNS lookups, and you have methods for examining the results. I 
> > have a couple of critiques of the presented API:
> > 
> > * ast_dns_resolve_async_recurring() is not very well defined and there are 
> > no examples illustrating its use.
> > * The function ast_dns_query_set_get() creates a leaky abstraction for the 
> > query set. I suggest one of the following:
> > * Have ast_dns_query_set_add() return an integer "token" that can be 
> > used to retrieve that record from the query set. (enthusiasm level: meh)
> > * Have an ast_dns_query_set iterator API to iterate over the queries in 
> > the set. (enthusiasm level: YES)
> > * While there are ways of querying if a result is secure, there is no 
> > current way of requesting only secure results.
> > 
> > From a low-level, the one big thing that I feel is not talked about is the 
> > threading model. I know that's getting really low level, but there are some 
> > questions that came to mind, such as: if I perform multiple asynchronous 
> > DNS lookups (without using a query set) can I guarantee the results will be 
> > presented to me serially, or can the results be presented in separate 
> > threads at the same time? This can have an impact on how I write my async 
> > callbacks, especially if I pass a reference to the same user data to both 
> > async queries.
> > 
> > The low-level API looks fine, and it provides a lot of areas where some 
> > higher-level functions could be created. For instance:
> > * NAPTR has a bunch of interesting possibilities:
> > * Have a function to automatically perform the regex replacement and 
> > present the result to you as a string.
> > * Or if you want to be even lazier, have a NAPTR function that will 
> > take a NAPTR result and convert that into an ast_dns_query that you can 
> > then resolve yourself.
> > * Or if you want to be even lazier, have an async NAPTR function not 
> > return results until it boils down to an A or  record.
> > * NAPTR and SRV have the potential for functions that allow for you to 
> > iterate over the different priority records that are returned.
> > * NAPTR and SRV could have fallback functions built into them, too.
> > 
> > As far as the examples are concerned, they're good, but I feel like an 
> > example that uses NAPTR (which subsequently results in an SRV lookup) could 
> > be useful. You may find that when you write the example, manual 
> > construction of further queries based on the data that you are retrieving 
> > from NAPTR records leads you to want to implement some of the suggested 
> > high-level functions for it.
> > 
> > Regarding your question about DNS being core or module-based, it kind of 
> > depends on a few factors:
> > * What code in Asterisk is going to be updated to use the new DNS API? If 
> > older code is not going to be updated to use the new DNS API, then I think 
> > that resolvers should live in loadable modules. If someone upgrades to 
> > Asterisk 15 and plans to continue using chan_sip and hasn't had issues with 
> > DNS, then they probably aren't going to be too thrilled about having to 
> > download c-ares or libunbound, only to not actually use it for anything. If 
> > old code is going to be updated to use the new DNS API, then...
> > * Are we planning on keeping any of the old DNS code from Asterisk around 
> > as a resolver implementation choice? If so, then again, I suggest having 
> > resolvers be separate modules. This way we still provide an upgrade path 
> > that does not have new dependencies or new underlying implementations.
> > 
> > My personal opinion is that all old code should be updated to use the new 
> > DNS API (without actually changing behavior, even if the old behavior is 
> > incorrect) and that the old implementation should be thrown away. If we go 
> > that route, then resolvers should be part of the core since DNS resolution 
> > is a fundamental thing in most Asterisk installations.
> > 
> > There is one further thing I can think of here, and that is the current 
> > dnsmgr code in Asterisk. Is that simply going to be updated to use the new 
> > DNS API, or is dnsmgr going to get some sort of overhaul to provide new 
> > functionality that the underlying DNS API makes it capable of?
> 
> Matt Jordan wrote:
> {quote}
> Regarding your question about DNS being core or module-based, it kind of 
> depends on a few factors:
> * What code in Asterisk is going to be updated to use the new DNS API? If 
> older code is not going to be updated to use the new DNS API, then I think 
> that resolvers should live in loadable modules. If someone upgrades to 
> Asterisk 15 and plans to continue using chan_sip a

Re: [asterisk-dev] [Code Review] 4437: dns: Define a core DNS API with examples.

[Matt Jordan]

> On Feb. 24, 2015, 5:48 p.m., Mark Michelson wrote:
> > First off, good page! It's pretty easy to follow the expected flow for DNS 
> > queries based on the API you've provided. From a low-level, there isn't 
> > much that's missing: you have ways of performing synchronous and 
> > asynchronous DNS lookups, and you have methods for examining the results. I 
> > have a couple of critiques of the presented API:
> > 
> > * ast_dns_resolve_async_recurring() is not very well defined and there are 
> > no examples illustrating its use.
> > * The function ast_dns_query_set_get() creates a leaky abstraction for the 
> > query set. I suggest one of the following:
> > * Have ast_dns_query_set_add() return an integer "token" that can be 
> > used to retrieve that record from the query set. (enthusiasm level: meh)
> > * Have an ast_dns_query_set iterator API to iterate over the queries in 
> > the set. (enthusiasm level: YES)
> > * While there are ways of querying if a result is secure, there is no 
> > current way of requesting only secure results.
> > 
> > From a low-level, the one big thing that I feel is not talked about is the 
> > threading model. I know that's getting really low level, but there are some 
> > questions that came to mind, such as: if I perform multiple asynchronous 
> > DNS lookups (without using a query set) can I guarantee the results will be 
> > presented to me serially, or can the results be presented in separate 
> > threads at the same time? This can have an impact on how I write my async 
> > callbacks, especially if I pass a reference to the same user data to both 
> > async queries.
> > 
> > The low-level API looks fine, and it provides a lot of areas where some 
> > higher-level functions could be created. For instance:
> > * NAPTR has a bunch of interesting possibilities:
> > * Have a function to automatically perform the regex replacement and 
> > present the result to you as a string.
> > * Or if you want to be even lazier, have a NAPTR function that will 
> > take a NAPTR result and convert that into an ast_dns_query that you can 
> > then resolve yourself.
> > * Or if you want to be even lazier, have an async NAPTR function not 
> > return results until it boils down to an A or  record.
> > * NAPTR and SRV have the potential for functions that allow for you to 
> > iterate over the different priority records that are returned.
> > * NAPTR and SRV could have fallback functions built into them, too.
> > 
> > As far as the examples are concerned, they're good, but I feel like an 
> > example that uses NAPTR (which subsequently results in an SRV lookup) could 
> > be useful. You may find that when you write the example, manual 
> > construction of further queries based on the data that you are retrieving 
> > from NAPTR records leads you to want to implement some of the suggested 
> > high-level functions for it.
> > 
> > Regarding your question about DNS being core or module-based, it kind of 
> > depends on a few factors:
> > * What code in Asterisk is going to be updated to use the new DNS API? If 
> > older code is not going to be updated to use the new DNS API, then I think 
> > that resolvers should live in loadable modules. If someone upgrades to 
> > Asterisk 15 and plans to continue using chan_sip and hasn't had issues with 
> > DNS, then they probably aren't going to be too thrilled about having to 
> > download c-ares or libunbound, only to not actually use it for anything. If 
> > old code is going to be updated to use the new DNS API, then...
> > * Are we planning on keeping any of the old DNS code from Asterisk around 
> > as a resolver implementation choice? If so, then again, I suggest having 
> > resolvers be separate modules. This way we still provide an upgrade path 
> > that does not have new dependencies or new underlying implementations.
> > 
> > My personal opinion is that all old code should be updated to use the new 
> > DNS API (without actually changing behavior, even if the old behavior is 
> > incorrect) and that the old implementation should be thrown away. If we go 
> > that route, then resolvers should be part of the core since DNS resolution 
> > is a fundamental thing in most Asterisk installations.
> > 
> > There is one further thing I can think of here, and that is the current 
> > dnsmgr code in Asterisk. Is that simply going to be updated to use the new 
> > DNS API, or is dnsmgr going to get some sort of overhaul to provide new 
> > functionality that the underlying DNS API makes it capable of?

{quote}
Regarding your question about DNS being core or module-based, it kind of 
depends on a few factors:
* What code in Asterisk is going to be updated to use the new DNS API? If older 
code is not going to be updated to use the new DNS API, then I think that 
resolvers should live in loadable modules. If someone upgrades to Asterisk 15 
and plans to continue using chan_sip and hasn't had issues with DNS, then they 
probably

Re: [asterisk-dev] [Code Review] 4437: dns: Define a core DNS API with examples.

[Joshua Colp]

James Cloos wrote:

"J" == Joshua Colp  writes:


J>  2. Are there any higher level APIs that would be useful to have?

In addition to the dns_naptr.h and dns_srv.h apis, there should be a
similar dns_tlsa.h api.

Even if tlsa is not yet widespread, Asterisk's dns code should offer
quality support for it.  And Asterisk should use them.


I've added this as a comment on the review so it's all nicely together 
if someone looks at things.


--
Joshua Colp
Digium, Inc. | Senior Software Developer
445 Jan Davis Drive NW - Huntsville, AL 35806 - US
Check us out at: www.digium.com & www.asterisk.org

--
_
-- 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

Re: [asterisk-dev] [Code Review] 4437: dns: Define a core DNS API with examples.

[Joshua Colp]

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


>From James Cloos on the mailing list:

In addition to the dns_naptr.h and dns_srv.h apis, there should be a
similar dns_tlsa.h api.

Even if tlsa is not yet widespread, Asterisk's dns code should offer
quality support for it.  And Asterisk should use them.

- Joshua Colp


On Feb. 23, 2015, 12:25 a.m., Joshua Colp wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4437/
> ---
> 
> (Updated Feb. 23, 2015, 12:25 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> A wiki page is present at:
> 
> https://wiki.asterisk.org/wiki/display/~jcolp/Asterisk+DNS+API
> 
> Which details a new DNS API for Asterisk. This exists as a thin wrapper over 
> other resolver libraries. The core part provides a common interface and some 
> additional features, such as NAPTR/SRV parsing and recurring lookups. 
> Examples are provided which cover the common use cases for the API.
> 
> Some stuff to think about:
> 
> 1. Does this encompass everything we think a low level API should?
> 2. Are there any higher level APIs that would be useful to have?
> 3. Is the usage intuitive and easy?
> 4. Are there other examples which would help?
> 5. Do we want resolvers to be actual modules or keep them in-core?
> 6. Anything else you think of
> 
> Have at it!
> 
> 
> Diffs
> -
> 
> 
> Diff: https://reviewboard.asterisk.org/r/4437/diff/
> 
> 
> Testing
> ---
> 
> I've logically run through the API and examples to ensure they provide what 
> is needed for the future, to make them as easy as possible to use, and to 
> ensure higher level APIs can be created.
> 
> 
> Thanks,
> 
> Joshua Colp
> 
>

-- 
_
-- 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