[codec] AD Evaluation of draft-ietf-codec-opus-update-07

Ben Campbell <ben@nostrum.com> Tue, 25 July 2017 22:34 UTC

Return-Path: <ben@nostrum.com>
X-Original-To: codec@ietfa.amsl.com
Delivered-To: codec@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 25391131FB2; Tue, 25 Jul 2017 15:34:17 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -1.881
X-Spam-Level:
X-Spam-Status: No, score=-1.881 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, RP_MATCHES_RCVD=-0.001, T_SPF_HELO_PERMERROR=0.01, T_SPF_PERMERROR=0.01] autolearn=ham autolearn_force=no
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 8yBIFdGVSgiy; Tue, 25 Jul 2017 15:34:15 -0700 (PDT)
Received: from nostrum.com (raven-v6.nostrum.com [IPv6:2001:470:d:1130::1]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 000BA131FB0; Tue, 25 Jul 2017 15:34:11 -0700 (PDT)
Received: from [10.0.1.63] (cpe-66-25-7-22.tx.res.rr.com [66.25.7.22]) (authenticated bits=0) by nostrum.com (8.15.2/8.15.2) with ESMTPSA id v6PMYAxE084850 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NO); Tue, 25 Jul 2017 17:34:11 -0500 (CDT) (envelope-from ben@nostrum.com)
X-Authentication-Warning: raven.nostrum.com: Host cpe-66-25-7-22.tx.res.rr.com [66.25.7.22] claimed to be [10.0.1.63]
From: Ben Campbell <ben@nostrum.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\))
Message-Id: <44ADD827-E40E-4BBE-91DB-EFFC249AA10E@nostrum.com>
Date: Tue, 25 Jul 2017 17:34:10 -0500
Cc: codec@ietf.org
To: draft-ietf-codec-opus-update.all@ietf.org
X-Mailer: Apple Mail (2.3273)
Archived-At: <https://mailarchive.ietf.org/arch/msg/codec/40czjXhmglKIQ3LROsmfWnfBCeE>
Subject: [codec] AD Evaluation of draft-ietf-codec-opus-update-07
X-BeenThere: codec@ietf.org
X-Mailman-Version: 2.1.22
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: Tue, 25 Jul 2017 22:34:17 -0000

Hi,

This is my AD evaluation of draft-ietf-codec-opus-update-07. I have a few material comments, and some nits. I’d like to address the material comments prior to IETF last call. The nits can be handled now, or later along with any IETF last call comments.

Thanks!

Ben.


Material Comments:

- Introduction:

—  I don’t think we can expect IETF LC and IESG reviewers to know the history of RFC 6716 going into this. Please expand the first paragraph in the introduction to  explain how the normative part of RFC 6716 is in the form of attached C code in appendix A, and that the patches in this draft patch that code.

— How can people be certain that the “formatted” patches match the patches in this draft, and do not change over time? Would it make sense to include a hash in this document, simularly to how 6716 has the hashes for the test vectors? (And are the IETF98 proceedings really where you want to store these?)

- 11: The draft should include hashes for the new test vectors, similarly to in 6716.

-12: The security considerations need some real content. For example, several of the fixes here seem to fix potential buffer overruns or other memory management errors. Does that reduce the attach surface for DoS or other attacks? I suspect the answer is “no”, since several of those sections mention that the bugs don’t appear to be exploitable. But the security considerations should at least summarize these sorts of changes and say why you believe they have no material impact on security.


Nits:

-3, last paragraph: Is no “significant” impact on test vectors the same as no impact? Also, I note that none of the other patch sections talk about test vector impact, and the section on new test vectors explains which patches impact the vectors. Could the comment in this section just be removed?

-5, list items 2 and 3:  These paragraphs (and others in the draft) mention symbols from the code without any explanation what they mean. Some of the symbol names are self-documenting, but that is not consistently true. It would be helpful to add short comments about the meaning of each of these (perhaps in parentheses). One might think of this as similar to expanding acronyms on first use.

Also, the use of underscores for emphasis is, to my knowledge, not meaningful in the context of an RFC. Is the emphasis really needed?  (I assume you don’t want to wait for the new RFC format for this sort of thing :-)  )

— paragraph after the list: "However, proving that is non-
   obvious.” - Odd sentence structure. Would it make sense to say “However, the authors know of no obvious approach to proving that.”?

-8: Please expand NaN on first mention.
-9: Please expand LCG on first mention