[FFmpeg-devel] [PATCH] aes: fix array index out of bounds warnings

[Måns Rullgård]

Reimar D?ffinger <Reimar.Doeffinger at gmx.de> writes:

> On Sat, Jun 26, 2010 at 09:12:52AM +0100, M?ns Rullg?rd wrote:
>> Reimar D?ffinger <Reimar.Doeffinger at gmx.de> writes:
>> 
>> > On Sat, Jun 26, 2010 at 12:54:21AM +0100, Mans Rullgard wrote:
>> >> ---
>> >>  libavutil/aes.c |    4 ++--
>> >>  1 files changed, 2 insertions(+), 2 deletions(-)
>> >> 
>> >> diff --git a/libavutil/aes.c b/libavutil/aes.c
>> >> index d3a271c..763f856 100644
>> >> --- a/libavutil/aes.c
>> >> +++ b/libavutil/aes.c
>> >> @@ -130,7 +130,7 @@ int av_aes_init(AVAES *a, const uint8_t *key, int key_bits, int decrypt) {
>> >>      uint8_t  log8[256];
>> >>      uint8_t alog8[512];
>> >>  
>> >> -    if(!enc_multbl[0][sizeof(enc_multbl)/sizeof(enc_multbl[0][0])-1]){
>> >> +    if(!enc_multbl[FF_ARRAY_ELEMS(enc_multbl)-1][FF_ARRAY_ELEMS(enc_multbl[0])-1]){
>> >>          j=1;
>> >>          for(i=0; i<255; i++){
>> >>              alog8[i]=
>> >> @@ -184,7 +184,7 @@ int av_aes_init(AVAES *a, const uint8_t *key, int key_bits, int decrypt) {
>> >>      }else{
>> >>          for(i=0; i<(rounds+1)>>1; i++){
>> >>              for(j=0; j<16; j++)
>> >> -                FFSWAP(int, a->round_key[i][0][j], a->round_key[rounds-i][0][j]);
>> >> +                FFSWAP(int, a->round_key[i][j>>2][j&3], a->round_key[rounds-i][j>>2][j&3]);
>> >
>> > If the compilers were any good this would be the same,
>> 
>> The existing code has UNDEFINED BEHAVIOUR from indexing outside an
>> array.  Talk about same has no meaning.
>
> I admit I didn't check the standard about that special case but I
> think the conclusion was that given the other constraints for a
> two-dimensional array there isn't a good reason for it not to
> work in any implementation.

The memory is as far as I can tell guaranteed to be laid out like
that, yes.  However, the standard is quite clear about out-of-array
accesses being undefined, which means the compiler might do just about
anything.  Compare to the cases where gcc determines a signed overflow
will occur and removes and entire branch of code, even though the
intuitive behaviour is obvious.

>> > but last time this was checked IIRC produced quite a lot slower
>> > code.
>> 
>> And that matters in an init function?  I also can't find anyone ever
>> claiming to have actually checked it.  All I do find is Michael
>> vehemently rejecting the notion of the out-of-array indexing being a
>> bug.
>
> Sorry, I didn't realize that. For AES the speed of the init function
> might be someonewhat relevant (I think it would matter the way it is misused
> as has etc. for BluRay), however this is even the encode init function.
> I retract my objection.

So can we just fix this bug and slash a page from the warnings in the
process?

-- 
M?ns Rullg?rd
mans at mansr.com