[Mesa-dev] [PATCH 2/2] draw: fix overflows in the indexed rendering paths

[Zack Rusin]

> On 07/03/2013 02:33 PM, Zack Rusin wrote:
> > ----- Original Message -----
> >> The code looks good AFAICT.  Just a few style nits below.
> >>
> >> For both:  Reviewed-by: Brian Paul <brianp at vmware.com>
> >
> > Thanks a lot for the review.
> >
> >
> >>>    #define DRAW_GET_IDX(_elts, _i)                   \
> >>> -   (((_i) >= draw->pt.user.eltMax) ? 0 : (_elts)[_i])
> >>> +   (((_i) >= draw->pt.user.eltMax) ? 0xffffffff : (_elts)[_i])
> >>
> >> replace 0xffffffff with MAX_ELT_IDX?
> >
> > Would you be ok with replacing all of those with UINT_MAX? Otherwise this
> > one should really be MAX_INDEX_BUFFER_IDX (maximum index in the index
> > buffer), the next one MAX_LOOP_IDX (max representable loop index), the
> > following one MAX_FETCH_IDX (max index of a vertex to fetch, which is what
> > the MAX_ELT_IDX currently represents), or we could call them all what they
> > really are which is maximum unsigned's or UINT_MAX.
> 
> I didn't realize that all the occurrences of 0xffffffff were different
> things.  I thought they were all the max vertex index.

Ah, yea, no, it's an overflow extravaganza. We can overflow: within the loop that splits primitives (e.g. if start vertex + count >= MAX_LOOP_IDX), we can overflow within the index buffer when we fetch beyond the size of the index buffer (MAX_ELT_IDX) and we can overflow if the vertex id fetched from the index buffer is larger than the size of the vertex buffer (MAX_FETCH_IDX). And that's before we even get to computing the actual place in the vertex buffer (which then can overflow if the vertex id, stride, buffer offset and format overflow, but that's already handled).

> I guess I'd vote for separate MAX_LOOP_IDX, MAX_FETCH_IDX, MAX_ELT_IDX
> symbols to help with code comprehension.

k, I'll use those