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

[Francisco Jerez]

Kenneth Graunke <kenneth at whitecape.org> writes:

> 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

That would definitely be a good thing to change regardless of whether we
go with one patch or the other.  bblock_t::prev/next return a bblock_t
pointer so the return value should better point to an object of type
bblock_t if non-null -- Otherwise you're encouraging people to make this
sort of mistakes.

> way you never return a pointer to the wrong type of memory.  This would
> become:
>
>       if (!prev_block)
>          continue;
>
Yeah, this hunk from my patch would also become unnecessary in that
case:

| -      bblock_t *prev_block = block->prev();
| +      bblock_t *const prev_block = block->num ? block->prev() : NULL;

> 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.

The reason the null checks are unnecessary is that the instruction being
an ENDIF or ELSE guarantees that there is a preceding instruction, so it
seems redundant to verify that prev_inst is non-NULL in addition.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160330/60b4e074/attachment.sig>