Re: [PATCH 4/4] hw/display/artist.c: fix out of bounds check

[Helge Deller]

On 29.07.20 19:26, Richard Henderson wrote:
> On 7/27/20 2:46 PM, Helge Deller wrote:
>> -        for (i = 0; i < pix_count; i++) {
>> +        for (i = 0; i < pix_count && offset + i < buf->size; i++) {
>>              artist_rop8(s, p + offset + pix_count - 1 - i,
>>                          (data & 1) ? (s->plane_mask >> 24) : 0);
>>              data >>= 1;
>
> This doesn't look right.
>
> You're writing to "offset + pix_count - 1 - i" and yet you're checking bounds
> vs "offset + i".
>
> This could be fixed by computing the complete offset into a local variable and
> then have an inner if to avoid the write, as you do for the second loop.
>
> But it would be better to precompute the correct loop bounds.

Thanks for the feedback.
Will send out a revised version soon.

Helge

>
> r~
>
>
>> @@ -398,7 +390,9 @@ static void vram_bit_write(ARTISTState *s, int posx, int 
>> posy, bool incr_x,
>>          for (i = 3; i >= 0; i--) {
>>              if (!(s->image_bitmap_op & 0x20000000) ||
>>                  s->vram_bitmask & (1 << (28 + i))) {
>> -                artist_rop8(s, p + offset + 3 - i, data8[ROP8OFF(i)]);
>> +                if (offset + 3 - i < buf->size) {
>> +                    artist_rop8(s, p + offset + 3 - i, data8[ROP8OFF(i)]);
>> +                }
>>              }
>>          }
>>          memory_region_set_dirty(&buf->mr, offset, 3);
>> @@ -420,7 +414,7 @@ static void vram_bit_write(ARTISTState *s, int posx, int 
>> posy, bool incr_x,
>>              break;
>>          }
>>
>> -        for (i = 0; i < pix_count; i++) {
>> +        for (i = 0; i < pix_count && offset + i < buf->size; i++) {
>>              mask = 1 << (pix_count - 1 - i);
>>
>>              if (!(s->image_bitmap_op & 0x20000000) ||