[Mesa-dev] [PATCH] radeon: fixed division by zero

[Jakob Sinclair]

On 2016-05-18 16:08, Nicolai Hähnle wrote:
> On 18.05.2016 08:17, Jakob Sinclair wrote:
>> In the function r600_test_dma the max_width and max_height can be set 
>> to
>> zero.
> 
> Thanks for looking into that. Can they really? I see
> 
> max_width = MIN2(tsrc.width0, tdst.width0);
> max_height = MIN2(tsrc.height0, tdst.height0);
> 
> and
> 
> tsrc.width0 = (rand() % max_tex_side_gen) + 1;
> 
> and similarly for tdst.
> 
> Given that, I'd prefer instead an assert(max_width > 0 && max_height >
> 0) to help Coverity out.
> 
> Thanks,
> Nicolai
> 
>> These variables are later used to do a modulo operation which can
>> lead to unexpected behavior if they are zero. This patch corrects that
>> by first checking if the max_width or max_height are zero and if they
>> are it sets width or height to one. Issue was discoverd by Coverity.
>> 
>> CID: 1361542, 1361543
>> 
>> Signed-off-by: Jakob Sinclair <sinclair.jakob at openmailbox.org>
>> ---
>>   src/gallium/drivers/radeon/r600_test_dma.c | 11 +++++++++--
>>   1 file changed, 9 insertions(+), 2 deletions(-)
>> 
>> diff --git a/src/gallium/drivers/radeon/r600_test_dma.c 
>> b/src/gallium/drivers/radeon/r600_test_dma.c
>> index c203b4d..37d6202 100644
>> --- a/src/gallium/drivers/radeon/r600_test_dma.c
>> +++ b/src/gallium/drivers/radeon/r600_test_dma.c
>> @@ -345,8 +345,15 @@ void r600_test_dma(struct r600_common_screen 
>> *rscreen)
>>   					dstx = rand() % (tdst.width0 - width + 1) & ~0x7;
>>   					dsty = rand() % (tdst.height0 - height + 1) & ~0x7;
>>   				} else {
>> -					width = (rand() % max_width) + 1;
>> -					height = (rand() % max_height) + 1;
>> +					if (max_width == 0)
>> +						width = 1;
>> +					else
>> +						width = (rand() % max_width) + 1;
>> +
>> +					if (max_height == 0)
>> +						height = 1;
>> +					else
>> +						height = (rand() % max_height) + 1;
>> 
>>   					srcx = rand() % (tsrc.width0 - width + 1);
>>   					srcy = rand() % (tsrc.height0 - height + 1);
>> 

After looking over it one more time it seems that you are right. I think 
the reason why Coverity is confused is because it thinks that we imply 
that tsrc.width0 can be 0 because we do a check if it is smaller than 
tdst.width0 which can have the value 1. If tdst.width0 can be 1 it 
basicly thinks that tsrc.width0 can be 0 because why else would we check 
that it can be smaller. Doing it your way will probably silence the 
Coverity warning. Thanks for the feedback, will soon post V2.

-- 
Mvh Jakob Sinclair