Subject: Re: [payload] AD review of draft-ietf-payload-g7110-02

From: Richard Barnes <[email-masked]>
Date: Thu, 19 Jun 2014 10:07:27 -0400

Hi Michael,

Sorry for the delay.  Responses inline (RLB).

On Mon, May 26, 2014 at 10:23 PM, Michael Ramalho (mramalho) <
[email-masked]> wrote:

>  Richard,
>
>
>
> My apologies, I didn’t see your email.
>
>
>
> I had a death in the family on May 10 (Saturday) and I wasn’t made aware
> of your email (which you sent the Monday afterwards) until recently.
>
>
>
> My comments are below (prefaced with “MAR:”). I have copied the co-authors
> on the draft for their commentary (they can comment via email separately).
>
>
>
> Thank you very much for your very thorough review. It was my belief that
> the LC ended, but I can see value in incorporating some of your comments.
>
>
>
> Regards,
>
>
>
> Michael Ramalho
>
>
>
> *From:* payload [mailto:[email-masked]] *On Behalf Of *Richard
> Barnes
> *Sent:* Monday, May 12, 2014 4:01 PM
> *To:* [email-masked]; [email-masked]
> *Subject:* [payload] AD review of draft-ietf-payload-g7110-02
>
>
>
> I have reviewed this draft in preparation for IETF LC.  I have some
> concerns below that I would like to see addressed before LC (MAJOR).
>
>
>
> Thanks,
>
> --Richard
>
>
>
>
>
> MAJOR:
>
>
>
> S4.2.3.
>
> H2 is a little unclear.  Is the array indexing zero-based?  If so, why are
> you skipping the first octet?  If you mean it to be zero-based, and not to
> skip the first octet, set P=-1 in H1.
>
>
>
> MAR: Noboru may have another way to address your point (and can comment in
> another email), but P, K and N are counters as defined in the previous
> paragraphs – not array indexes.
>
>
>
> MAR: H2 says to read the minimum of 321 or (N-P) octets into an internal
> buffer from the (P+1)st octet. Since P starts at zero in H1, we never skip
> the first octet; on the first pass the first octet is placed in the
> internal buffer.
>

So you appear to be asserting that if you view the payload as an array, it
is indexed starting at 1.   If I had a normal, zero-based array, the "(P+1)
octet of the payload" would actually be array[P], not array[P+1].  This
seems likely to create interop problems.  How about adding the following
clarification:

"(In the common case of a zero-based array, this will read octets P through
P+min{320+1, (N-P)})"

>  S4.2.3.
>
> What happens in H5 if the G.711.0 decoder encounters an improperly
> formatted octet sequence?  E.g., if the prefix code octet specifies that
> there are more samples than could be contained in the packet.  I'm
> concerned that the process as-is could lead to reading past the end of the
> packet buffer.
>
>
>
> MAR: For a given pass through H5, the internal buffer will at most have
> all the remaining octets in the RTP payload by virtue of the “Read min
> {321,(N-P)} octets” in H2. Stated another way, there is no way the internal
> buffer can have more octets than was originally in the RTP payload.
>
>
>
> MAR: If the ITU-T specified decoder could (due to bit error) read past the
> end of the data intended in the internal buffer, it would decode the
> (apparently) random data after that to the end of the internal buffer into
> something – but that something would be of limited length by virtue of the
> decoding process. This point is also made in the Security Considerations
> section – that data posing as a desired G.711.0 payload could not be turned
> into a denial-of-service attack (as the G.711.0 processing has a worst case
> processing path and completes in a finite amount of processing, no loops,
> etc.).
>
>
>
> MAR: If we really, really, really want to ensure that the G.711.0 decoder
> didn’t even read junk in the internal buffer past the intended G.711.0 data
> – we could modify the “Read min {321,(N-P)} octexts” in H2 to explicitly
> state that any unused octets in the 321 byte internal buffer after the read
> must be set to 0x00 (which would be guaranteed not to produce any
> unintended G.711 source symbols). We could easily do this by modifying H2
> to read:
>
>
>
> Read internal buffer: “Set all octets in the internal buffer to zero
> (0x00), then read min(321,(N-P)} octexts  into the internal buffer from the
> (P+1) octet in the RTP Payload”.
>
>
>
> MAR: I would defer to Noboru here.
>

So, my concern was that the length processed by the G.711.0 decoder would
cause it to read past the end of the *internal* buffer, not the packet.
 (Sorry for the confusion.)  But I had missed the fact that the longest
read that a G.711.0 length octet can specify is 320 bits.  From that, and
from your comments, it seems like the worst case is that the decoder reads
old samples, as opposed to unallocated memory.

Could we add the following to the security considerations?
"The length of data read by the G.711.0 encoder is controlled by the length
octet in the encoded G.711.0 frame.  The algorithm above ensures that the
G.711.0 encoder will not read beyond the internal buffer.  The internal
buffer is specified to be 321 octets, and longest length that can be
represented in the G.711.0 prefix code is 320 octets."

>  S4.2.4.
>
> What happens if the M is not a multiple of N?  Does the receiver discard
> the packet?
>
>
>
> MAR: Good point. Of course this should never happen with a correctly
> formatted payload, but it wouldn’t hurt to explicitly mention that it is
> probably wise to discard the packet.
>
>
>
> MAR: As an aside, you should have reviewed RFC 3551 – as it is silent on
> the point of what happens if there aren’t enough samples to complete a
> sample-based encoding (Section 4.3).
>
>
>
> MAR: Assuming my co-authors and WG chairs agree, we can draft such a
> statement.
>

How about the following:

OLD: "We note that if both parameters channels and ptime are known that one
could provide a check for the other and the converse."
NEW: "We note that if both parameters channels and ptime are known that one
could provide a check for the other and the converse.  Whichever algorithm
is used to determine the number of channels, if the length of payload (N)
is not divisible by the number of channels (M), then the packet SHOULD be
discarded."

>  MINOR:
>
>
>
> S4.2.3.
>
> The "heuristic" seems more like an "algorithm".  Or "process", as the
> section header says.
>
>
>
> MAR: If my co-authors and WG chairs agree, I would be happy to change
> heuristic to process at all relevant occurrences.
>
>
>
> S5.1.
>
> Is there any risk of G.7110 being defined, and needing a payload format
> that would collide with this identifier?
>
>
>
> MAR: Paul Jones (ITU-T SG16 Rapporteur and co-author here) can comment …
> but the G-series numbering ran out a while ago and they began to label
> things with a “dot” nomenclature (e.g., G.722.1). Thus I doubt that there
> will be a G.7110 standard in the G series for media codecs.
>
>
>
> S5.1.
>
> Can we specify that "channels" is an integer?
>
>
>
> MAR: I have no issue, but I can’t find any mention in RFC4566 or any other
> RTP format definition that explicitly mentions non-integer channels.
>
>
>
> MAR: “5.1 surround” isn’t really 5.1 channels ;-) ).
>
>
>
> S5.4.*
>
> Can someone verify that the spaces in the examples are valid?  E.g., "complaw
> = al" and "ptime: 20".
>
>
>
> S6.
>
> It seems like the format described here can only hold single-channel,
> right?  If so, this format seems fine, but you do need to clarify that it's
> single-channel only. If not, it seems like you need to (1) delimit packets,
> and (2) specify the number of channels at the start.
>
>
>
> MAR: Yes, the storage mode defined in Section 6.3 with version octet
> “0x00” only supports a single channel.
>
>
>
> MAR: As the text describes, if there is a future need for other formats to
> support things like multiple channels or metadata … they can be defined
> elsewhere (in another IETF document or in another SDO) with a version
> number other than “0x00”.
>

How about this?

OLD: "... essentially a concatenation of individual G.711.0 frames."
NEW: "... essentially a concatenation of individual G.711.0 frames.  Note
that although the above payload format supports multiple channels, this
storage format supports only a single channel."

>   EDITORIAL:
>
>
>
> S4.2.4.
>
> "the optional parameter is a MUST" -> "the optional 'channels' parameter
> is REQUIRED"
>
>
>
> MAR: Good catch.
>
>
>
> S4.2.3.
>
> It seems like it would be simpler to state the decoding algorithm as while
> loop with an if statement, since it seems like that's how you would
> implement it in most normal programming languages.  (See, e.g., the python
> snippet below).  If you really want a state machine like representation, it
> would be nice to at least have a picture of it.
>
>
>
>       +-----------+
>
>       |           |
>
>       V           |
>
> H1 -> H2 -> H3 -> H4 -> STOP
>
>       ^     |            ^
>
>       |     |            |
>
>       |     +---> H5 ----+
>
>       |           |
>
>       |           |
>
>       +-----------+
>
>
>
> def decode_g711_0_payload(payload):
>
>   # H1
>
>   P = 0
>
>   N = len(payload)
>
>   internal = []
>
>   output = []
>
>   while P < N:
>
>     # H2
>
>     internal = packet[ P+1 : P+1+min(321,N-P) ]
>
>     # H3
>
>     if internal[0] == '\x00':
>
>       # H4
>
>       P += 1
>
>     else:
>
>       # H5
>
>       (symbols, Q) = g711_0_decode(internal)
>
>       output.extend(symbols)
>
>       K += len(symbols) # == M
>
>       P += Q
>
>   return output
>
>
>
> MAR: I wrote the prose more or less to mimic the words in the ITU-T
> G.711.0 specification.
>
>
>
> MAR: However, I would have no issue with a picture or pseudo code. I
> completely defer to Paul Jones or Noboru Harada here.
>
>
>
> MAR: My most sincere thanks for your through review.
>

It's up to you.  Please make sure the code is correct, though!  The above
code is not correct, since it makes the "P vs P+1" mistake I was worried
about above.

--Richard