Re: [codec] Stephen Farrell's No Objection on draft-ietf-codec-oggopus-13: (with COMMENT)
"Timothy B. Terriberry" <tterribe@xiph.org> Wed, 17 February 2016 02:11 UTC
Return-Path: <tterribe@xiph.org>
X-Original-To: codec@ietfa.amsl.com
Delivered-To: codec@ietfa.amsl.com
Received: from localhost (ietfa.amsl.com [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 6484D1B2AF7; Tue, 16 Feb 2016 18:11:19 -0800 (PST)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -2.613
X-Spam-Level:
X-Spam-Status: No, score=-2.613 tagged_above=-999 required=5 tests=[BAYES_50=0.8, HELO_MISMATCH_ORG=0.611, HOST_MISMATCH_COM=0.311, RCVD_IN_DNSWL_HI=-5, SPF_SOFTFAIL=0.665] autolearn=ham
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id BEo3QpXgrnHB; Tue, 16 Feb 2016 18:11:14 -0800 (PST)
Received: from smtp.mozilla.org (mx2.scl3.mozilla.com [63.245.214.156]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 2BB931AC409; Tue, 16 Feb 2016 18:11:14 -0800 (PST)
Received: from localhost (localhost6.localdomain [127.0.0.1]) by mx2.mail.scl3.mozilla.com (Postfix) with ESMTP id 8BCD0C128D; Wed, 17 Feb 2016 02:11:12 +0000 (UTC)
X-Virus-Scanned: amavisd-new at mozilla.org
Received: from smtp.mozilla.org ([127.0.0.1]) by localhost (mx2.mail.scl3.mozilla.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id LRVWgKE6JTgo; Wed, 17 Feb 2016 02:11:12 +0000 (UTC)
Received: from [10.252.26.22] (corp.mtv2.mozilla.com [63.245.221.32]) (Authenticated sender: tterriberry@mozilla.com) by mx2.mail.scl3.mozilla.com (Postfix) with ESMTPSA id 5FD45C0531; Wed, 17 Feb 2016 02:11:12 +0000 (UTC)
Message-ID: <56C3D6C0.70904@xiph.org>
Date: Tue, 16 Feb 2016 18:11:12 -0800
From: "Timothy B. Terriberry" <tterribe@xiph.org>
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:29.0) Gecko/20100101 SeaMonkey/2.26
MIME-Version: 1.0
To: Stephen Farrell <stephen.farrell@cs.tcd.ie>, The IESG <iesg@ietf.org>
References: <20160215180233.30650.39279.idtracker@ietfa.amsl.com>
In-Reply-To: <20160215180233.30650.39279.idtracker@ietfa.amsl.com>
Content-Type: multipart/mixed; boundary="------------050606040502040502050105"
Archived-At: <http://mailarchive.ietf.org/arch/msg/codec/IvOk7VVoZ3VdywnLJvblLhSJSFU>
Cc: codec-chairs@ietf.org, codec@ietf.org, draft-ietf-codec-oggopus@ietf.org
Subject: Re: [codec] Stephen Farrell's No Objection on draft-ietf-codec-oggopus-13: (with COMMENT)
X-BeenThere: codec@ietf.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: Codec WG <codec.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/codec>, <mailto:codec-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/codec/>
List-Post: <mailto:codec@ietf.org>
List-Help: <mailto:codec-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/codec>, <mailto:codec-request@ietf.org?subject=subscribe>
X-List-Received-Date: Wed, 17 Feb 2016 02:11:19 -0000
Thanks for the detailed comments. Responses are in-line below. Stephen Farrell wrote: > - general, (but a nit): there are some odd unexplained numbers > here, which is fine ... but odd. (E.g. 125,829,120) It might be > nicer to explain to the reader why these values were chosen. I The number quoted is just 120 MB (120*1024*1024). I added a parenthetical to clarify this. The 61,440 number in the same paragraph is explained later in the text. Happy to clarify any others you think should be explained better. > - An example or diagram in the intro or section 3 would maybe > have helped. The appropriate figure for the introduction is probably the one from Section 5 of RFC 3533. We could copy it here, but I think the reference is enough (it would probably require more text to explain it, text which is already in RFC 3533). Section 3 is a bit more specific to this draft. I added the following figure: Page 1 Pages 2 ... n Pages (n+1) ... +------------+ +---+ +---+ ... +---+ +-----------+ +---------+ +-- | | | | | | | | | | | | | |+----------+| |+-----------------+| |+-------------------+ +----- |||ID Header|| || Comment Header || ||Audio Data Packet 1| | ... |+----------+| |+-----------------+| |+-------------------+ +----- | | | | | | | | | | | | | +------------+ +---+ +---+ ... +---+ +-----------+ +---------+ +-- ^ ^ ^ | | | | | Mandatory page break | | | ID header is contained on a single page | 'Beggining Of Stream' Figure 1: Example packet organization for a logical Ogg Opus stream > - section 5: Q7.8 is a new one to me. Maybe add a reference? Added a reference to <https://en.wikipedia.org/w/index.php?title=Q_%28number_format%29&oldid=697252615>. > - 5.1.1.5: Could figures 3-8 do with some body text to explain > them? That's (having body text that refers to the figures), > general good practice and I'm not sure if they're sufficiently > clear for an implementer to get right. (But as I'm not coding > this, I don't know, so just checking:-) I think the meaning is relatively unambiguous, but certainly adding a reference to the figures instead of just saying "the following matrices" would be clearer, so I have done that. > - 5.2: You don't say that the user comment strings must also be > UTF8, but you do for the vendor string. Why not? I think it'd be > good to call that out. The text doesn't but the header says: "User Comment #i String (variable length, UTF-8 vector):" I've normalized the text to match that of the vendor string for consistency, however. > - 5.2.1: [vorbis-comment] is, correctly, a normative reference > here but I wonder if a xiph.org URL is a good enough reference > for that. Is there anything better, or are we confident enough > in that URL? According to archive.org, that URL has been stable since at least 2005 (since the year 2000, according to our svn logs, but the final version for Vorbis 1.0 wasn't posted until July 11th 2002). I don't know of a better authority. > - 5.2.1: From what namespace are the -573 and 111 values here > selected? How is that managed? (Just wondering.) Those values are just examples. I've added a sentence saying that, since it must not have been clear. The syntax for them is described in the paragraph below. If people feel that description is imprecise we could probably add some EBNF. > - section 9: While I like the MUST NOT here, it's really only > wishful thinking and isn't a strictly valid use of 2119 terms. > I'm ok with that though. The SHOULD NOT statement is also kind > of bogus. Generally this section would be better if it had > guidance on where implementers are likely to go wrong in ways > that cause security issues. Reference such as are provided in I agree. The current text was adapted from RFC 6716, but I think we have enough implementation experience at this point to write something better. I propose the following additional text: Header parsing code contains the most likely area for potential overruns. It is important for implementations to ensure their buffers contain enough data for all of the required fields before attempting to read it (for example, for all of the channel map data in the ID header). Implementations would do well to validate the indices of the channel map, also, to ensure they meet all of the restrictions outlined in Section 5.1.1, in order to avoid attempting to read data from channels that do not exist. To avoid excessive resource usage, we advise implementations to be especially wary of streams that might cause them to process far more data than was actually transmitted. For example, a relatively small comment header may contain values for the string lengths or user comment list length that imply that it is many gigabytes in size. Even computing the size of the required buffer could overflow a 32-bit integer, and actually attempting to allocate such a buffer before verifying it would be a reasonable size is a bad idea. After reading the user comment list length, implementations might wish to verify that the header contains at least the minimum amount of data for that many comments (4 additional octets per comment, to indicate each has a length of zero) before proceeding any further, again taking care to avoid overflow in these calculations. If allocating an array of pointers to point at these strings, the size of the pointers may be larger than 4 octets, potentially requiring a separate overflow check. Another bug in this class we have observed more than once involves the handling of invalid data at the end of a stream. Often, implementations will seek to the end of a stream to locate the last timestamp in order to compute its total duration. If they do not find a valid capture pattern and Ogg page from the desired logical stream, they will back up and try again. If care is not taken to avoid re-scanning data that was already scanned, this search can quickly devolve into something with a complexity that is quadratic in the amount of invalid data. In general when seeking, implementations will wish to be cautious about the effects of invalid granule position values, and ensure all algorithms will continue to make progress and eventually terminate, even if these are missing or out-of-order. > RFC 6562 about the dangers of VBR might also be useful here, not > sure. In most of the expected uses of Ogg Opus on the internet (e.g., https streaming), the encryption and network packetization would be separate from the container's packetization/pagination, so I don't think the dangers referred to by RFC 6562 apply (if you can see the Ogg packet sizes, it's because you already have access to the decrypted plaintext). In other contexts, RFC 6716 already includes a reference to 6562. I've attached a diff of the relevant changes to the XML.
- [codec] Stephen Farrell's No Objection on draft-i… Stephen Farrell
- Re: [codec] Stephen Farrell's No Objection on dra… Timothy B. Terriberry
- Re: [codec] Stephen Farrell's No Objection on dra… Stephen Farrell