[opus] [PATCH] Support for Channel Mapping 253.

[Mark Harris]

Hi Drew,

Thanks for the fixes.  See below for a couple of remaining issues.


On Mon, Nov 20, 2017 at 5:57 PM, Drew Allen <bitllama at google.com> wrote:
> On Sat, Nov 18, 2017 at 5:48 PM Mark Harris <mark.hsj at gmail.com> wrote:
>>
>>  5) opus_projection_decoder_init: demixing_matrix_size_in_bytes
>> doesn't include the MappingMatrix struct or alignment but is used to
>> determine the offset of the decoder; it should be obtained from
>> mapping_matrix_get_size() and the field should be opus_int32.
>>
> This value is only meant to represent the "data" portion of the
> mappingmatrix struct. Note that we include the struct size when computing
> the multistream decoder pointer in get_multistream_decoder().
> I will make it opus_int32 though.

Where in get_multistream_decoder() does it include the size of struct
MappingMatrix?  It only includes sizeof(OpusProjectionDecoder) and
st->demixing_matrix_size_in_bytes, neither of which includes the size
of struct MappingMatrix.  And if you're not going to use
mapping_matrix_get_size() then you'll also need to include the
alignment padding after the matrix data separately from alignment
padding on each of the two structs.

>
>>
>>  7) opus_projection_ambisonics_encoder_init: It appears that
>> uninitialized values are used from the matrices if order_plus_one is 1
>> or 5 to 15.
>
> I will rename the function that validates the correct order before this is
> called (It fails and hence the init() func fails if order_plus_one is < 2 or
>> 4.)

Where does it check for order_plus_one < 2 or > 4?  The draft allows
order 0 and also higher orders.  The current check in
src/opus_projection_encoder.c line 65 checks for "order_plus_one < 1
|| order_plus_one > 15".  I don't see a more restrictive check, and if
there is one somewhere it doesn't prevent
opus_projection_ambisonics_encoder_init() from accessing uninitialized
memory.  This can be seen by running test_opus_projection under
valgrind:

==22926== Use of uninitialised value of size 8
==22926==    at 0x1000E61D0: opus_projection_ambisonics_encoder_init
(opus_projection_encoder.c:197)
==22926==    by 0x1000E642C: opus_projection_ambisonics_encoder_create
(opus_projection_encoder.c:247)
==22926==    by 0x1000014C1: test_creation_arguments
(test_opus_projection.c:235)
==22926==    by 0x100001AEE: main (test_opus_projection.c:411)
==22926==  Uninitialised value was created by a heap allocation
==22926==    at 0x10009B616: malloc (vg_replace_malloc.c:302)
==22926==    by 0x1000E6407: opus_projection_ambisonics_encoder_create
(os_support.h:49)
==22926==    by 0x1000014C1: test_creation_arguments
(test_opus_projection.c:235)
==22926==    by 0x100001AEE: main (test_opus_projection.c:411)
==22926==
==22926== Conditional jump or move depends on uninitialised value(s)
==22926==    at 0x1000E61E5: opus_projection_ambisonics_encoder_init
(opus_projection_encoder.c:204)
==22926==    by 0x1000E642C: opus_projection_ambisonics_encoder_create
(opus_projection_encoder.c:247)
==22926==    by 0x1000014C1: test_creation_arguments
(test_opus_projection.c:235)
==22926==    by 0x100001AEE: main (test_opus_projection.c:411)
==22926==  Uninitialised value was created by a heap allocation
==22926==    at 0x10009B616: malloc (vg_replace_malloc.c:302)
==22926==    by 0x1000E6407: opus_projection_ambisonics_encoder_create
(os_support.h:49)
==22926==    by 0x1000014C1: test_creation_arguments
(test_opus_projection.c:235)
==22926==    by 0x100001AEE: main (test_opus_projection.c:411)

 - Mark