<div dir="ltr">Hey Mark,<div><br></div><div>My apologies...Attached is a revised patch that corrects those issues.</div><div><br></div><div>Cheers,</div><div>Drew</div><br><div class="gmail_quote"><div dir="ltr">On Mon, Nov 20, 2017 at 9:17 PM Mark Harris <<a href="mailto:mark.hsj@gmail.com" target="_blank">mark.hsj@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Drew,<br>
<br>
Thanks for the fixes.  See below for a couple of remaining issues.<br>
<br>
<br>
On Mon, Nov 20, 2017 at 5:57 PM, Drew Allen <<a href="mailto:bitllama@google.com" target="_blank">bitllama@google.com</a>> wrote:<br>
> On Sat, Nov 18, 2017 at 5:48 PM Mark Harris <<a href="mailto:mark.hsj@gmail.com" target="_blank">mark.hsj@gmail.com</a>> wrote:<br>
>><br>
>>  5) opus_projection_decoder_init: demixing_matrix_size_in_bytes<br>
>> doesn't include the MappingMatrix struct or alignment but is used to<br>
>> determine the offset of the decoder; it should be obtained from<br>
>> mapping_matrix_get_size() and the field should be opus_int32.<br>
>><br>
> This value is only meant to represent the "data" portion of the<br>
> mappingmatrix struct. Note that we include the struct size when computing<br>
> the multistream decoder pointer in get_multistream_decoder().<br>
> I will make it opus_int32 though.<br>
<br>
Where in get_multistream_decoder() does it include the size of struct<br>
MappingMatrix?  It only includes sizeof(OpusProjectionDecoder) and<br>
st->demixing_matrix_size_in_bytes, neither of which includes the size<br>
of struct MappingMatrix.  And if you're not going to use<br>
mapping_matrix_get_size() then you'll also need to include the<br>
alignment padding after the matrix data separately from alignment<br>
padding on each of the two structs.<br>
<br>
><br>
>><br>
>>  7) opus_projection_ambisonics_encoder_init: It appears that<br>
>> uninitialized values are used from the matrices if order_plus_one is 1<br>
>> or 5 to 15.<br>
><br>
> I will rename the function that validates the correct order before this is<br>
> called (It fails and hence the init() func fails if order_plus_one is < 2 or<br>
>> 4.)<br>
<br>
Where does it check for order_plus_one < 2 or > 4?  The draft allows<br>
order 0 and also higher orders.  The current check in<br>
src/opus_projection_encoder.c line 65 checks for "order_plus_one < 1<br>
|| order_plus_one > 15".  I don't see a more restrictive check, and if<br>
there is one somewhere it doesn't prevent<br>
opus_projection_ambisonics_encoder_init() from accessing uninitialized<br>
memory.  This can be seen by running test_opus_projection under<br>
valgrind:<br>
<br>
==22926== Use of uninitialised value of size 8<br>
==22926==    at 0x1000E61D0: opus_projection_ambisonics_encoder_init<br>
(opus_projection_encoder.c:197)<br>
==22926==    by 0x1000E642C: opus_projection_ambisonics_encoder_create<br>
(opus_projection_encoder.c:247)<br>
==22926==    by 0x1000014C1: test_creation_arguments<br>
(test_opus_projection.c:235)<br>
==22926==    by 0x100001AEE: main (test_opus_projection.c:411)<br>
==22926==  Uninitialised value was created by a heap allocation<br>
==22926==    at 0x10009B616: malloc (vg_replace_malloc.c:302)<br>
==22926==    by 0x1000E6407: opus_projection_ambisonics_encoder_create<br>
(os_support.h:49)<br>
==22926==    by 0x1000014C1: test_creation_arguments<br>
(test_opus_projection.c:235)<br>
==22926==    by 0x100001AEE: main (test_opus_projection.c:411)<br>
==22926==<br>
==22926== Conditional jump or move depends on uninitialised value(s)<br>
==22926==    at 0x1000E61E5: opus_projection_ambisonics_encoder_init<br>
(opus_projection_encoder.c:204)<br>
==22926==    by 0x1000E642C: opus_projection_ambisonics_encoder_create<br>
(opus_projection_encoder.c:247)<br>
==22926==    by 0x1000014C1: test_creation_arguments<br>
(test_opus_projection.c:235)<br>
==22926==    by 0x100001AEE: main (test_opus_projection.c:411)<br>
==22926==  Uninitialised value was created by a heap allocation<br>
==22926==    at 0x10009B616: malloc (vg_replace_malloc.c:302)<br>
==22926==    by 0x1000E6407: opus_projection_ambisonics_encoder_create<br>
(os_support.h:49)<br>
==22926==    by 0x1000014C1: test_creation_arguments<br>
(test_opus_projection.c:235)<br>
==22926==    by 0x100001AEE: main (test_opus_projection.c:411)<br>
<br>
 - Mark<br>
</blockquote></div></div>