Re: Bug: reading out of bounds in cint_array.c

[arnold]

Michael,

Thanks for the report.

Andy - please push that to master (after checking make check and
make valgrind).

Thanks to both of you!

Arnold

"Andrew J. Schorr" <address@hidden> wrote:

> Hi,
>
> On Mon, Jan 13, 2020 at 04:32:29PM +0300, Michael Builov wrote:
> > I have just spotted a possible read beyond array bounds in
> > cint_array_init().
> > What if NHAT is defined as 30 in the environment?
> > 
> > The code:
> > 
> > static NODE **
> > cint_array_init(NODE *symbol, NODE *subs)
> > {
> > ........
> >         /* check relevant environment variables */
> >         if ((newval = getenv_long("NHAT")) > 1 && newval < INT32_BIT)
> >             NHAT = (unsigned) newval;
> >         /* don't allow overflow off the end of the table */
> >         if (NHAT >= nelems)
> >             NHAT = nelems - 2;
> >         THRESHOLD = power_two_table[NHAT + 1];
> > ..........
> > }
> > 
> > INT32_BIT == 32
> > nelems == 31
> > so
> > THRESHOLD = power_two_table[30 + 1];   // reading out of bounds!
>
> Thanks for the bug report. I think your analysis is correct.
> This patch should fix it:
>
> diff --git a/cint_array.c b/cint_array.c
> index 417f27d..d7171ac 100644
> --- a/cint_array.c
> +++ b/cint_array.c
> @@ -171,15 +171,15 @@ cint_array_init(NODE *symbol ATTRIBUTE_UNUSED, NODE 
> *subs ATTRIBUTE_UNUSED)
>               long newval;
>               size_t nelems = (sizeof(power_two_table) / 
> sizeof(power_two_table[0]));
>  
>               /* check relevant environment variables */
>               if ((newval = getenv_long("NHAT")) > 1 && newval < INT32_BIT)
>                       NHAT = newval;
>               /* don't allow overflow off the end of the table */
> -             if (NHAT >= nelems)
> +             if (NHAT > nelems - 2)
>                       NHAT = nelems - 2;
>               THRESHOLD = power_two_table[NHAT + 1];
>       } else
>               null_array(symbol);
>  
>       return & success_node;
>  }
>
>
> Unless there's a deeper logic flaw elsewhere related to NHAT usage. I haven't
> reviewed whether there was some twisted and mistaken logic behind the original
> error that might point to problems elsewhere...
>
> Regards,
> Andy