[Asterisk-code-review] rtp engine: Allow more than 32 dynamic payload types. (asterisk[13])

[Alexander Traud]

Alexander Traud has posted comments on this change.

Change subject: rtp_engine: Allow more than 32 dynamic payload types.
......................................................................


Patch Set 1:

> Asterisk should be able to have 100 different dynamic payload types registered, and then assign a payload number on the fly when a call is made/received.

I am not sure, I understand this. There are not 100 but only 32 types in the dynamic range. Asterisk 14.1 is going to use *all* of them (thanks to the recent Signed Linear, SILK, and Codec 2 additions). When the user goes for allow=all, the limit of 32 is still a hard one. Consequently, having a dynamic allocation of the payload numbers while a call is created – although that would be nice – is orthogonal to the change here.

> configuration option

Please, when you propose something be as concrete as possible, even a stomach-feeling example helps me to understand the scenario better. For example, when you propose a configuration option simply make up a name (and if feasible mention its configuration file). Even if it is a first shot, that helps me to work it out, determine what you are looking for and what style you use for configuration options. For me as external contributor this is very complicated because I have to look at all other options understanding their style and avoiding review ping pong. I do not have a bird's eye view on the project.

> the concerns are justified

Nobody was about un-justifying a statement. The problem was and is to understand the concern. Therefore, when you share the same point of view, instead of a Me Too, please re-phrase the concern in different terms. That might help to understand the concern better.

> an implementation may fail the SDP negotiation, resulting in the call failing.

OK, I think I got it. I re-iterate the issue, please, correct me where I am wrong.

The MAY is just for the caller (Asterisk). The callee has to support this scenario, anything else would be a software bug in the callee. I see five scenarios; the callee might
A) ignore such a re-assigned format, or
B) use the assigned (wrong) format because it ignores the rtpmap, or
C) refuse the whole call because the SDP is not as expected, or
D) stale/crash because the SDP is not expected, or
E) everything works fine.
Case A to D are software bugs. Case D is not our issue and revealing it would be good actually, because an implementation must be guarded against chaos/random input and is not allowed to crash/stale. Case A is not an issue because the format would have not been offered without this change here at all. There is no behavior change. Case B is avoided because no already assigned value is re-assigned. No behavior change. Consequently, case C is the concern you are about. To trigger case C, the implementation must contain a malicious double-check for the lower bound of the rtpmap. It must divert its known formats into well-known (static) assigned and dynamically assigned ones, and check each dynamic one against a minimum RTP Payload Type number of 96.

Or stated differently: The implementation must contain extra code to trigger the raised concern here. The Asterisk Team does not want a call failure but rather a format failure.

That is OK for me. However, first I had to understand your concern. For this, I learned to go through the possible cases and in the next step the possibly results. In the last step, I envision how that result+case happens. I learned these three steps to analyze a risk. It is great that you can do such risk-analysis in one step thanks to your experience. However please, others (not experienced like you) have to understand and follow your reasoning.

I added the configuration option and added a text to the CHANGES document. Better wordings for the description in the configuration sample and CHANGES file are welcome. As well as a better name/place for the configuration option. If you need a Log message, please say so (which level, suggested wording, and whether only at start of Asterisk or for each call).

-- 
To view, visit https://gerrit.asterisk.org/3681
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7bc96ab764bc30098a178b841cbf7146f9d64964
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Alexander Traud <pabstraud at compuserve.com>
Gerrit-Reviewer: Alexander Traud <pabstraud at compuserve.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-HasComments: No