[Mesa-dev] [PATCH] i965: Fix invalid pointer read in dead_control_flow_eliminate().

[Kenneth Graunke]

On Wednesday, March 30, 2016 1:24:09 PM PDT Francisco Jerez wrote:
> Kenneth Graunke <kenneth at whitecape.org> writes:
> 
> > There may not be a previous block.  In this case, there's no real work
> > to do, so just continue on to the next one.
> >
> > Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> > ---
> >  src/mesa/drivers/dri/i965/brw_dead_control_flow.cpp | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_dead_control_flow.cpp b/src/
mesa/drivers/dri/i965/brw_dead_control_flow.cpp
> > index 2c1abaf..116a6c7 100644
> > --- a/src/mesa/drivers/dri/i965/brw_dead_control_flow.cpp
> > +++ b/src/mesa/drivers/dri/i965/brw_dead_control_flow.cpp
> > @@ -42,6 +42,10 @@ dead_control_flow_eliminate(backend_shader *s)
> >  
> >     foreach_block_safe (block, s->cfg) {
> >        bblock_t *prev_block = block->prev();
> > +
> > +      if (prev_block->link.is_head_sentinel())
> > +         continue;
> > +
> Heh, the fact this code declares a "prev_block" pointer of type bblock_t
> and then asks the object whether it actually is a bblock_t really makes
> me itch -- If it's not a bblock_t you're likely relying on UB (At least
> the strict aliasing rule seems to be violated).

Yeah, I don't like it either.

I also wrote a follow-on patch that makes block->prev(), block->next(),
check for head/tail sentinels and return a NULL bblock_t pointer.  That
way you never return a pointer to the wrong type of memory.  This would
become:

      if (!prev_block)
         continue;

I feel like that's the least surprising API.  What do you think?

> I sent a patch to address the same issue a couple of weeks ago which
> doesn't have this problem.  It's still pending review:
> 
> https://patchwork.freedesktop.org/patch/77199/

I thought about doing that too, but I wasn't convinced at a glance that
NULL'ing the pointers would work.  We dereference prev_inst below
without checking if it's NULL.  I think the inst->opcode checks
guarantee it's safe.  But I thought that simply skipping over the
"no previous block" case with a continue was clearer.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160330/8717c5b5/attachment.sig>