GNOME Bugzilla – Bug 773207
audio-converter: incorrect use of memcpy for in-place conversion in do_unpack()
Last modified: 2016-10-26 17:09:49 UTC
If in-place unpack is allowed and the format is the default format the do_unpack() function calls memcpy with source and destination being the same pointer. This is forbidden by the library standard: "The memcpy() function copies n bytes from memory area src to memory area dest. The memory areas must not overlap. Use memmove(3) if the memory areas do overlap." Therefore memmove should be used instead. The code snippet from do_unpack() : if (in_writable && chain->allow_ip) { tmp = convert->in_data; GST_LOG ("unpack in-place %p, %" G_GSIZE_FORMAT, tmp, num_samples); } else { ... } if (convert->in_data) { for (i = 0; i < chain->blocks; i++) { if (convert->in_default) { GST_LOG ("copy %p, %p, %" G_GSIZE_FORMAT, tmp[i], convert->in_data[i], num_samples); memcpy (tmp[i], convert->in_data[i], num_samples * chain->stride); } else { ... } } } else { ...
The same issue is potentially in converter_passthrough() if called with out==in. But there it's not clear to me if this case can happen at all. For any case it would be useful to either also use memmove there or to mention in a comment that out must not equal in.
(In reply to Petr Kulhavy from comment #1) > The same issue is potentially in converter_passthrough() if called with > out==in. But there it's not clear to me if this case can happen at all. For > any case it would be useful to either also use memmove there or to mention > in a comment that out must not equal in. Or an assertion instead of a comment to actually enforce it. Do you plan to provide patches for these two problems?
Yes, I will provide the patches. I guess in the do_unpack() this situation can happen, please correct me if I'm wrong. And then the memcpy can be actually just skipped, because it copies the data over itself. In converter_passthrough() it probably never happens, my experience shows that it is not called at all. So I will put an assert there. I just don't want to break something due to my ignorance...
Created attachment 338012 [details] [review] Patch for converter_passthrough() This is the patch with assert in converter_passthrough()
Created attachment 338013 [details] [review] This is a patch for do_unpack() I've realized the check is already there. The memcpy is never called with dst==src. So just add a comment to explicitly point it out.
Comment on attachment 338013 [details] [review] This is a patch for do_unpack() Adding an assertion for the memcpy() would still seem useful here, or don't you think? Also just adding that to gst_audio_converter_samples() would make sense IMHO
(In reply to Sebastian Dröge (slomo) from comment #6) > Adding an assertion for the memcpy() would still seem useful here, or don't > you think? It is actually not needed. The long if: if (!chain->allow_ip || !in_writable || !convert->in_default) takes care of that. The condition is equal to !(chain->allow_ip && in_writable && convert->in_default) which is exactly the only case when the memcpy() is called with dst==src.
Yes I mean making exactly this explicit in the memcpy() branch. Someone might add code there later which breaks this assumption otherwise :)
Created attachment 338108 [details] [review] Patch for do_unpack() - version 2 with assertion Now I see what you mean. That makes sense. Attaching a new patch. I also moved the if out of the loop, which will slightly speed up the processing for non-interleaved channels.
With the changes in https://bugzilla.gnome.org/show_bug.cgi?id=773073 the patch for converter passthrough is no longer relevant.
*** This bug has been marked as a duplicate of bug 773073 ***