| Message ID | 20170701100552.17049-1-onemda@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Sat, Jul 01, 2017 at 12:05:52PM +0200, Paul B Mahol wrote: > Signed-off-by: Paul B Mahol <onemda@gmail.com> > --- > libavcodec/mlz.c | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/libavcodec/mlz.c b/libavcodec/mlz.c > index ebce796..715ea5c 100644 > --- a/libavcodec/mlz.c > +++ b/libavcodec/mlz.c > @@ -112,12 +112,7 @@ static int decode_string(MLZ* mlz, unsigned char *buff, int string_code, int *fi > } > > static int input_code(GetBitContext* gb, int len) { > - int tmp_code = 0; > - int i; > - for (i = 0; i < len; ++i) { > - tmp_code |= get_bits1(gb) << i; > - } > - return tmp_code; > + return get_bitsz(gb, len); have you tested this ? (seems not triggered in fate, i ve only found fuzzed samples that trigger this with len >= 1 quickly) isnt this the opposite endianness ? [...]
On 7/1/17, Michael Niedermayer <michael@niedermayer.cc> wrote: > On Sat, Jul 01, 2017 at 12:05:52PM +0200, Paul B Mahol wrote: >> Signed-off-by: Paul B Mahol <onemda@gmail.com> >> --- >> libavcodec/mlz.c | 7 +------ >> 1 file changed, 1 insertion(+), 6 deletions(-) >> >> diff --git a/libavcodec/mlz.c b/libavcodec/mlz.c >> index ebce796..715ea5c 100644 >> --- a/libavcodec/mlz.c >> +++ b/libavcodec/mlz.c >> @@ -112,12 +112,7 @@ static int decode_string(MLZ* mlz, unsigned char >> *buff, int string_code, int *fi >> } >> >> static int input_code(GetBitContext* gb, int len) { >> - int tmp_code = 0; >> - int i; >> - for (i = 0; i < len; ++i) { >> - tmp_code |= get_bits1(gb) << i; >> - } >> - return tmp_code; >> + return get_bitsz(gb, len); > > have you tested this ? (seems not triggered in fate, i ve only found > fuzzed samples that trigger this with len >= 1 quickly) > isnt this the opposite endianness ? I created file with 22rev2 als encoder and decoding produce same hash, before and after this patch. ALS uses big endian bit reader.
On Sat, Jul 01, 2017 at 02:18:17PM +0200, Paul B Mahol wrote: > On 7/1/17, Michael Niedermayer <michael@niedermayer.cc> wrote: > > On Sat, Jul 01, 2017 at 12:05:52PM +0200, Paul B Mahol wrote: > >> Signed-off-by: Paul B Mahol <onemda@gmail.com> > >> --- > >> libavcodec/mlz.c | 7 +------ > >> 1 file changed, 1 insertion(+), 6 deletions(-) > >> > >> diff --git a/libavcodec/mlz.c b/libavcodec/mlz.c > >> index ebce796..715ea5c 100644 > >> --- a/libavcodec/mlz.c > >> +++ b/libavcodec/mlz.c > >> @@ -112,12 +112,7 @@ static int decode_string(MLZ* mlz, unsigned char > >> *buff, int string_code, int *fi > >> } > >> > >> static int input_code(GetBitContext* gb, int len) { > >> - int tmp_code = 0; > >> - int i; > >> - for (i = 0; i < len; ++i) { > >> - tmp_code |= get_bits1(gb) << i; > >> - } > >> - return tmp_code; > >> + return get_bitsz(gb, len); > > > > have you tested this ? (seems not triggered in fate, i ve only found > > fuzzed samples that trigger this with len >= 1 quickly) > > isnt this the opposite endianness ? > > I created file with 22rev2 als encoder and decoding produce same hash, > before and after this patch. > > ALS uses big endian bit reader. ok, but please change the commit message then as the code before and afterwards dont read in the same endianness so this is not a simplification but a bugfix then [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Everything should be made as simple as possible, but not simpler. -- Albert Einstein
On 7/1/17, Michael Niedermayer <michael@niedermayer.cc> wrote: > On Sat, Jul 01, 2017 at 02:18:17PM +0200, Paul B Mahol wrote: >> On 7/1/17, Michael Niedermayer <michael@niedermayer.cc> wrote: >> > On Sat, Jul 01, 2017 at 12:05:52PM +0200, Paul B Mahol wrote: >> >> Signed-off-by: Paul B Mahol <onemda@gmail.com> >> >> --- >> >> libavcodec/mlz.c | 7 +------ >> >> 1 file changed, 1 insertion(+), 6 deletions(-) >> >> >> >> diff --git a/libavcodec/mlz.c b/libavcodec/mlz.c >> >> index ebce796..715ea5c 100644 >> >> --- a/libavcodec/mlz.c >> >> +++ b/libavcodec/mlz.c >> >> @@ -112,12 +112,7 @@ static int decode_string(MLZ* mlz, unsigned char >> >> *buff, int string_code, int *fi >> >> } >> >> >> >> static int input_code(GetBitContext* gb, int len) { >> >> - int tmp_code = 0; >> >> - int i; >> >> - for (i = 0; i < len; ++i) { >> >> - tmp_code |= get_bits1(gb) << i; >> >> - } >> >> - return tmp_code; >> >> + return get_bitsz(gb, len); >> > >> > have you tested this ? (seems not triggered in fate, i ve only found >> > fuzzed samples that trigger this with len >= 1 quickly) >> > isnt this the opposite endianness ? >> >> I created file with 22rev2 als encoder and decoding produce same hash, >> before and after this patch. >> >> ALS uses big endian bit reader. > > ok, but please change the commit message then as the code before and > afterwards dont read in the same endianness so this is not a > simplification but a bugfix then Hmm, on second look you are probably right. They changed to this in R23 version and I failed to create file that triggers this path. So I will not apply this mostly because R23 support needs more work.
Am 01.07.17 um 14:42 schrieb Paul B Mahol: > On 7/1/17, Michael Niedermayer <michael@niedermayer.cc> wrote: >> On Sat, Jul 01, 2017 at 02:18:17PM +0200, Paul B Mahol wrote: >>> On 7/1/17, Michael Niedermayer <michael@niedermayer.cc> wrote: >>>> On Sat, Jul 01, 2017 at 12:05:52PM +0200, Paul B Mahol wrote: >>>>> Signed-off-by: Paul B Mahol <onemda@gmail.com> >>>>> --- >>>>> libavcodec/mlz.c | 7 +------ >>>>> 1 file changed, 1 insertion(+), 6 deletions(-) >>>>> >>>>> diff --git a/libavcodec/mlz.c b/libavcodec/mlz.c >>>>> index ebce796..715ea5c 100644 >>>>> --- a/libavcodec/mlz.c >>>>> +++ b/libavcodec/mlz.c >>>>> @@ -112,12 +112,7 @@ static int decode_string(MLZ* mlz, unsigned char >>>>> *buff, int string_code, int *fi >>>>> } >>>>> >>>>> static int input_code(GetBitContext* gb, int len) { >>>>> - int tmp_code = 0; >>>>> - int i; >>>>> - for (i = 0; i < len; ++i) { >>>>> - tmp_code |= get_bits1(gb) << i; >>>>> - } >>>>> - return tmp_code; >>>>> + return get_bitsz(gb, len); >>>> >>>> have you tested this ? (seems not triggered in fate, i ve only found >>>> fuzzed samples that trigger this with len >= 1 quickly) >>>> isnt this the opposite endianness ? >>> >>> I created file with 22rev2 als encoder and decoding produce same hash, >>> before and after this patch. >>> >>> ALS uses big endian bit reader. >> >> ok, but please change the commit message then as the code before and >> afterwards dont read in the same endianness so this is not a >> simplification but a bugfix then > > Hmm, on second look you are probably right. > They changed to this in R23 version and I failed to create file that > triggers this path. > So I will not apply this mostly because R23 support needs more work. Do I understand correctly, this patch works with RM22 only and fails on RM23? -Thilo
On 7/1/17, Thilo Borgmann <thilo.borgmann@mail.de> wrote: > Am 01.07.17 um 14:42 schrieb Paul B Mahol: >> On 7/1/17, Michael Niedermayer <michael@niedermayer.cc> wrote: >>> On Sat, Jul 01, 2017 at 02:18:17PM +0200, Paul B Mahol wrote: >>>> On 7/1/17, Michael Niedermayer <michael@niedermayer.cc> wrote: >>>>> On Sat, Jul 01, 2017 at 12:05:52PM +0200, Paul B Mahol wrote: >>>>>> Signed-off-by: Paul B Mahol <onemda@gmail.com> >>>>>> --- >>>>>> libavcodec/mlz.c | 7 +------ >>>>>> 1 file changed, 1 insertion(+), 6 deletions(-) >>>>>> >>>>>> diff --git a/libavcodec/mlz.c b/libavcodec/mlz.c >>>>>> index ebce796..715ea5c 100644 >>>>>> --- a/libavcodec/mlz.c >>>>>> +++ b/libavcodec/mlz.c >>>>>> @@ -112,12 +112,7 @@ static int decode_string(MLZ* mlz, unsigned char >>>>>> *buff, int string_code, int *fi >>>>>> } >>>>>> >>>>>> static int input_code(GetBitContext* gb, int len) { >>>>>> - int tmp_code = 0; >>>>>> - int i; >>>>>> - for (i = 0; i < len; ++i) { >>>>>> - tmp_code |= get_bits1(gb) << i; >>>>>> - } >>>>>> - return tmp_code; >>>>>> + return get_bitsz(gb, len); >>>>> >>>>> have you tested this ? (seems not triggered in fate, i ve only found >>>>> fuzzed samples that trigger this with len >= 1 quickly) >>>>> isnt this the opposite endianness ? >>>> >>>> I created file with 22rev2 als encoder and decoding produce same hash, >>>> before and after this patch. >>>> >>>> ALS uses big endian bit reader. >>> >>> ok, but please change the commit message then as the code before and >>> afterwards dont read in the same endianness so this is not a >>> simplification but a bugfix then >> >> Hmm, on second look you are probably right. >> They changed to this in R23 version and I failed to create file that >> triggers this path. >> So I will not apply this mostly because R23 support needs more work. > > Do I understand correctly, this patch works with RM22 only and fails on > RM23? Probably. I can not confirm because I failed to create sample with path that triggers this code.
Am 01.07.17 um 19:03 schrieb Paul B Mahol: > On 7/1/17, Thilo Borgmann <thilo.borgmann@mail.de> wrote: >> Am 01.07.17 um 14:42 schrieb Paul B Mahol: >>> On 7/1/17, Michael Niedermayer <michael@niedermayer.cc> wrote: >>>> On Sat, Jul 01, 2017 at 02:18:17PM +0200, Paul B Mahol wrote: >>>>> On 7/1/17, Michael Niedermayer <michael@niedermayer.cc> wrote: >>>>>> On Sat, Jul 01, 2017 at 12:05:52PM +0200, Paul B Mahol wrote: >>>>>>> Signed-off-by: Paul B Mahol <onemda@gmail.com> >>>>>>> --- >>>>>>> libavcodec/mlz.c | 7 +------ >>>>>>> 1 file changed, 1 insertion(+), 6 deletions(-) >>>>>>> >>>>>>> diff --git a/libavcodec/mlz.c b/libavcodec/mlz.c >>>>>>> index ebce796..715ea5c 100644 >>>>>>> --- a/libavcodec/mlz.c >>>>>>> +++ b/libavcodec/mlz.c >>>>>>> @@ -112,12 +112,7 @@ static int decode_string(MLZ* mlz, unsigned char >>>>>>> *buff, int string_code, int *fi >>>>>>> } >>>>>>> >>>>>>> static int input_code(GetBitContext* gb, int len) { >>>>>>> - int tmp_code = 0; >>>>>>> - int i; >>>>>>> - for (i = 0; i < len; ++i) { >>>>>>> - tmp_code |= get_bits1(gb) << i; >>>>>>> - } >>>>>>> - return tmp_code; >>>>>>> + return get_bitsz(gb, len); >>>>>> >>>>>> have you tested this ? (seems not triggered in fate, i ve only found >>>>>> fuzzed samples that trigger this with len >= 1 quickly) >>>>>> isnt this the opposite endianness ? >>>>> >>>>> I created file with 22rev2 als encoder and decoding produce same hash, >>>>> before and after this patch. >>>>> >>>>> ALS uses big endian bit reader. >>>> >>>> ok, but please change the commit message then as the code before and >>>> afterwards dont read in the same endianness so this is not a >>>> simplification but a bugfix then >>> >>> Hmm, on second look you are probably right. >>> They changed to this in R23 version and I failed to create file that >>> triggers this path. >>> So I will not apply this mostly because R23 support needs more work. >> >> Do I understand correctly, this patch works with RM22 only and fails on >> RM23? > > Probably. I can not confirm because I failed to create sample with > path that triggers this code. Well it should get applied only if it could properly be tested for both endianness cases. -Thilo
On 7/2/17, Thilo Borgmann <thilo.borgmann@mail.de> wrote: > Am 01.07.17 um 19:03 schrieb Paul B Mahol: >> On 7/1/17, Thilo Borgmann <thilo.borgmann@mail.de> wrote: >>> Am 01.07.17 um 14:42 schrieb Paul B Mahol: >>>> On 7/1/17, Michael Niedermayer <michael@niedermayer.cc> wrote: >>>>> On Sat, Jul 01, 2017 at 02:18:17PM +0200, Paul B Mahol wrote: >>>>>> On 7/1/17, Michael Niedermayer <michael@niedermayer.cc> wrote: >>>>>>> On Sat, Jul 01, 2017 at 12:05:52PM +0200, Paul B Mahol wrote: >>>>>>>> Signed-off-by: Paul B Mahol <onemda@gmail.com> >>>>>>>> --- >>>>>>>> libavcodec/mlz.c | 7 +------ >>>>>>>> 1 file changed, 1 insertion(+), 6 deletions(-) >>>>>>>> >>>>>>>> diff --git a/libavcodec/mlz.c b/libavcodec/mlz.c >>>>>>>> index ebce796..715ea5c 100644 >>>>>>>> --- a/libavcodec/mlz.c >>>>>>>> +++ b/libavcodec/mlz.c >>>>>>>> @@ -112,12 +112,7 @@ static int decode_string(MLZ* mlz, unsigned >>>>>>>> char >>>>>>>> *buff, int string_code, int *fi >>>>>>>> } >>>>>>>> >>>>>>>> static int input_code(GetBitContext* gb, int len) { >>>>>>>> - int tmp_code = 0; >>>>>>>> - int i; >>>>>>>> - for (i = 0; i < len; ++i) { >>>>>>>> - tmp_code |= get_bits1(gb) << i; >>>>>>>> - } >>>>>>>> - return tmp_code; >>>>>>>> + return get_bitsz(gb, len); >>>>>>> >>>>>>> have you tested this ? (seems not triggered in fate, i ve only found >>>>>>> fuzzed samples that trigger this with len >= 1 quickly) >>>>>>> isnt this the opposite endianness ? >>>>>> >>>>>> I created file with 22rev2 als encoder and decoding produce same hash, >>>>>> before and after this patch. >>>>>> >>>>>> ALS uses big endian bit reader. >>>>> >>>>> ok, but please change the commit message then as the code before and >>>>> afterwards dont read in the same endianness so this is not a >>>>> simplification but a bugfix then >>>> >>>> Hmm, on second look you are probably right. >>>> They changed to this in R23 version and I failed to create file that >>>> triggers this path. >>>> So I will not apply this mostly because R23 support needs more work. >>> >>> Do I understand correctly, this patch works with RM22 only and fails on >>> RM23? >> >> Probably. I can not confirm because I failed to create sample with >> path that triggers this code. > > Well it should get applied only if it could properly be tested for both > endianness cases. Yes, until then I will not apply this. As float code works fine with R22rev2.
diff --git a/libavcodec/mlz.c b/libavcodec/mlz.c index ebce796..715ea5c 100644 --- a/libavcodec/mlz.c +++ b/libavcodec/mlz.c @@ -112,12 +112,7 @@ static int decode_string(MLZ* mlz, unsigned char *buff, int string_code, int *fi } static int input_code(GetBitContext* gb, int len) { - int tmp_code = 0; - int i; - for (i = 0; i < len; ++i) { - tmp_code |= get_bits1(gb) << i; - } - return tmp_code; + return get_bitsz(gb, len); } int ff_mlz_decompression(MLZ* mlz, GetBitContext* gb, int size, unsigned char *buff) {
Signed-off-by: Paul B Mahol <onemda@gmail.com> --- libavcodec/mlz.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)