Hi Mark, Attached are corrections based on your comments. I will extend these to the patch I recently submitted to fix memory issues once that is resolved as well. Cheers, Drew On Sat, Nov 18, 2017 at 5:48 PM Mark Harris <mark.hsj at gmail.com> wrote:> Hi Drew, > > Some additional comments on your mapping family 253 changes: > > 1) mapping_matrix_get_data: The computed pointer is not correct; > matrix should be cast to char * before the addition to avoid > incrementing in units of the structure size. > > Done.> 2) mapping_matrix_get_size: "rows * cols * sizeof(opus_int16)" may > not have the necessary alignment. There may be alignment padding > after the struct and also after the opus_int16 array and this function > is assumed to return a size that includes both. Also the > multiplication and return value should use opus_int32 since int may > not be large enough for order 11 to 14. A complication is that > align() currently takes and returns "int". > > We specify in the IETF doc that we are limited to a max matrix size of65025 bytes (and with 2 bytes per cell, that limits us to only order 11 for a full-order square matrix). I can recast align before summing them though.> 3) In several places including the intermediate multiplication in the > computation of "tmp" in mapping_matrix_multiply_short, and even in the > MATRIX_INDEX macro, multiplication may overflow if int is 16 bits. > One of the operands of the multiplication should be cast to opus_int32 > in such cases to ensure a 32-bit multiply and avoid overflow. > > Done.> 4) get_multistream_decoder: align() must be applied separately to > sizeof(OpusProjectionDecoder) and st->demixing_matrix_size_in_bytes > since they may each have padding. Including the alignment padding in > demixing_matrix_size_in_bytes as in (5) avoids the second call to > align(). Similarly in opus_projection_decoder_get_size, align() > should just be applied to sizeof(OpusProjectionDecoder); in that case > the other sizes may have their own alignment padding but it is already > included in their size assuming (2) is addressed. > > Done.> 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 themappingmatrix 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. 6) opus_projection_decoder_init: demixing_matrix_size is in bytes but> is used as a parameter to ALLOC() which expects the number of > elements, so twice as much memory is allocated. > > Done.> 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.)> > 8) opus_projection_ambisonics_encoder_init: > mixing_matrix_size_in_bytes and demixing_matrix_size_in_bytes should > be opus_int32.Done.> > - Mark >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.xiph.org/pipermail/opus/attachments/20171121/22db2c25/attachment.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: 0001-Resolve-align-issues-in-Projection-API.patch Type: application/octet-stream Size: 9733 bytes Desc: not available URL: <http://lists.xiph.org/pipermail/opus/attachments/20171121/22db2c25/attachment.obj>
Hi Drew, In which order do these patches apply? Jean-Marc On 11/20/2017 08:57 PM, Drew Allen wrote:> Hi Mark, > > Attached are corrections based on your comments. I will extend these to > the patch I recently submitted to fix memory issues once that is > resolved as well. > > Cheers, > Drew > > On Sat, Nov 18, 2017 at 5:48 PM Mark Harris <mark.hsj at gmail.com > <mailto:mark.hsj at gmail.com>> wrote: > > Hi Drew, > > Some additional comments on your mapping family 253 changes: > > 1) mapping_matrix_get_data: The computed pointer is not correct; > matrix should be cast to char * before the addition to avoid > incrementing in units of the structure size. > > Done. > > > 2) mapping_matrix_get_size: "rows * cols * sizeof(opus_int16)" may > not have the necessary alignment. There may be alignment padding > after the struct and also after the opus_int16 array and this function > is assumed to return a size that includes both. Also the > multiplication and return value should use opus_int32 since int may > not be large enough for order 11 to 14. A complication is that > align() currently takes and returns "int". > > We specify in the IETF doc that we are limited to a max matrix size of > 65025 bytes (and with 2 bytes per cell, that limits us to only order 11 > for a full-order square matrix). I can recast align before summing them > though. > > > 3) In several places including the intermediate multiplication in the > computation of "tmp" in mapping_matrix_multiply_short, and even in the > MATRIX_INDEX macro, multiplication may overflow if int is 16 bits. > One of the operands of the multiplication should be cast to opus_int32 > in such cases to ensure a 32-bit multiply and avoid overflow. > > Done. > > > 4) get_multistream_decoder: align() must be applied separately to > sizeof(OpusProjectionDecoder) and st->demixing_matrix_size_in_bytes > since they may each have padding. Including the alignment padding in > demixing_matrix_size_in_bytes as in (5) avoids the second call to > align(). Similarly in opus_projection_decoder_get_size, align() > should just be applied to sizeof(OpusProjectionDecoder); in that case > the other sizes may have their own alignment padding but it is already > included in their size assuming (2) is addressed. > > Done. > > 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. > > 6) opus_projection_decoder_init: demixing_matrix_size is in bytes but > is used as a parameter to ALLOC() which expects the number of > elements, so twice as much memory is allocated. > > Done. > > > 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.) > > > 8) opus_projection_ambisonics_encoder_init: > mixing_matrix_size_in_bytes and demixing_matrix_size_in_bytes should > be opus_int32. > > Done. > > > - Mark > > > > _______________________________________________ > opus mailing list > opus at xiph.org > http://lists.xiph.org/mailman/listinfo/opus >
Hi Jean-Marc, Both apply to master branch. I'll have to address Mark's comments on that latest memory patch as well. On Mon, Nov 20, 2017 at 6:45 PM Jean-Marc Valin <jmvalin at jmvalin.ca> wrote:> Hi Drew, > > In which order do these patches apply? > > Jean-Marc > > On 11/20/2017 08:57 PM, Drew Allen wrote: > > Hi Mark, > > > > Attached are corrections based on your comments. I will extend these to > > the patch I recently submitted to fix memory issues once that is > > resolved as well. > > > > Cheers, > > Drew > > > > On Sat, Nov 18, 2017 at 5:48 PM Mark Harris <mark.hsj at gmail.com > > <mailto:mark.hsj at gmail.com>> wrote: > > > > Hi Drew, > > > > Some additional comments on your mapping family 253 changes: > > > > 1) mapping_matrix_get_data: The computed pointer is not correct; > > matrix should be cast to char * before the addition to avoid > > incrementing in units of the structure size. > > > > Done. > > > > > > 2) mapping_matrix_get_size: "rows * cols * sizeof(opus_int16)" may > > not have the necessary alignment. There may be alignment padding > > after the struct and also after the opus_int16 array and this > function > > is assumed to return a size that includes both. Also the > > multiplication and return value should use opus_int32 since int may > > not be large enough for order 11 to 14. A complication is that > > align() currently takes and returns "int". > > > > We specify in the IETF doc that we are limited to a max matrix size of > > 65025 bytes (and with 2 bytes per cell, that limits us to only order 11 > > for a full-order square matrix). I can recast align before summing them > > though. > > > > > > 3) In several places including the intermediate multiplication in > the > > computation of "tmp" in mapping_matrix_multiply_short, and even in > the > > MATRIX_INDEX macro, multiplication may overflow if int is 16 bits. > > One of the operands of the multiplication should be cast to > opus_int32 > > in such cases to ensure a 32-bit multiply and avoid overflow. > > > > Done. > > > > > > 4) get_multistream_decoder: align() must be applied separately to > > sizeof(OpusProjectionDecoder) and st->demixing_matrix_size_in_bytes > > since they may each have padding. Including the alignment padding in > > demixing_matrix_size_in_bytes as in (5) avoids the second call to > > align(). Similarly in opus_projection_decoder_get_size, align() > > should just be applied to sizeof(OpusProjectionDecoder); in that case > > the other sizes may have their own alignment padding but it is > already > > included in their size assuming (2) is addressed. > > > > Done. > > > > 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. > > > > 6) opus_projection_decoder_init: demixing_matrix_size is in bytes > but > > is used as a parameter to ALLOC() which expects the number of > > elements, so twice as much memory is allocated. > > > > Done. > > > > > > 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.) > > > > > > 8) opus_projection_ambisonics_encoder_init: > > mixing_matrix_size_in_bytes and demixing_matrix_size_in_bytes should > > be opus_int32. > > > > Done. > > > > > > - Mark > > > > > > > > _______________________________________________ > > opus mailing list > > opus at xiph.org > > http://lists.xiph.org/mailman/listinfo/opus > > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.xiph.org/pipermail/opus/attachments/20171121/c4ecffb5/attachment.html>
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
Hey Mark, My apologies...Attached is a revised patch that corrects those issues. Cheers, Drew On Mon, Nov 20, 2017 at 9:17 PM Mark Harris <mark.hsj at gmail.com> wrote:> 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 >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.xiph.org/pipermail/opus/attachments/20171123/36e281b3/attachment-0001.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: 0001-Resolve-align-issues-in-Projection-API.patch Type: application/octet-stream Size: 10732 bytes Desc: not available URL: <http://lists.xiph.org/pipermail/opus/attachments/20171123/36e281b3/attachment-0001.obj>