[vlc-devel] [PATCH] contribs: lame: Add assertion to make sure fftenergy is not read OoB

[David Fuhrmann]

> Am 23.12.2019 um 15:36 schrieb Felix Paul Kühne <fkuehne at videolan.org>:
> 
> Hello,
> 
> Is there a reason why this wasn’t merged?


Hello Felix,

This was one proposal from me to narrow down the problem a bit, I am not sure if there is actually an out of bounds read happening here or not.

But in the meantime, Thomas proposed and merged a patch to disable asserts altogether, so this patch does not make much sense anymore as it is.
As far as I understand, I also see no reason at the moment to convert this to an abort() or similar.

BR. David


> 
> Thanks!
> 
> Felix
> 
>> On 2. Dec 2019, at 10:37, david.fuhrmann at gmail.com wrote:
>> 
>> From: David Fuhrmann <dfuhrmann at videolan.org>
>> 
>> On macOS (only), we see crashes on the assertion a couple of code
>> lines below:
>> 
>> FLOAT const el = fftenergy[j];
>> assert(el >= 0);
>> 
>> To narrow down the problem a bit, add a new assertion to make sure
>> fftenergy array is never read past its bound, which would lead to
>> undefined and potentially negative values.
>> ---
>> contrib/src/lame/invariant-for-energy-array.patch | 11 +++++++++++
>> contrib/src/lame/rules.mak                        |  4 ++++
>> 2 files changed, 15 insertions(+)
>> create mode 100644 contrib/src/lame/invariant-for-energy-array.patch
>> 
>> diff --git a/contrib/src/lame/invariant-for-energy-array.patch b/contrib/src/lame/invariant-for-energy-array.patch
>> new file mode 100644
>> index 000000000000..392a40380a5e
>> --- /dev/null
>> +++ b/contrib/src/lame/invariant-for-energy-array.patch
>> @@ -0,0 +1,11 @@
>> +--- lame/libmp3lame/psymodel.c.orig	2019-12-02 10:13:52.000000000 +0100
>> ++++ lame/libmp3lame/psymodel.c	2019-12-02 10:14:53.000000000 +0100
>> +@@ -571,6 +571,8 @@
>> +     for (b = j = 0; b < l->npart; ++b) {
>> +         FLOAT   ebb = 0, m = 0;
>> +         int     i;
>> ++
>> ++        assert(j + l->numlines[b] <= HBLKSIZE);
>> +         for (i = 0; i < l->numlines[b]; ++i, ++j) {
>> +             FLOAT const el = fftenergy[j];
>> +             assert(el >= 0);
>> diff --git a/contrib/src/lame/rules.mak b/contrib/src/lame/rules.mak
>> index 32827a8e69ac..50ce55625f89 100644
>> --- a/contrib/src/lame/rules.mak
>> +++ b/contrib/src/lame/rules.mak
>> @@ -15,6 +15,10 @@ lame: lame-$(LAME_VERSION).tar.gz .sum-lame
>> ifdef HAVE_VISUALSTUDIO
>> 	$(APPLY) $(SRC)/lame/struct-float-copy.patch
>> endif
>> +ifdef HAVE_DARWIN_OS
>> +	$(APPLY) $(SRC)/lame/invariant-for-energy-array.patch
>> +endif
>> +
>> 	# Avoid relying on iconv.m4 from gettext, when reconfiguring.
>> 	# This is only used by the frontend which we disable.
>> 	cd $(UNPACK_DIR) && sed -i.orig 's/^AM_ICONV/#&/' configure.in
>> -- 
>> 2.21.0 (Apple Git-122.2)
>> 
>> _______________________________________________
>> vlc-devel mailing list
>> To unsubscribe or modify your subscription options:
>> https://mailman.videolan.org/listinfo/vlc-devel
> 
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel