From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:48043)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <peter.maydell@linaro.org>) id 1b9tFm-0000Wc-K9
	for qemu-devel@nongnu.org; Mon, 06 Jun 2016 08:03:35 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <peter.maydell@linaro.org>) id 1b9tFl-0003lu-MP
	for qemu-devel@nongnu.org; Mon, 06 Jun 2016 08:03:34 -0400
Received: from mail-vk0-x22d.google.com ([2607:f8b0:400c:c05::22d]:33272)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <peter.maydell@linaro.org>) id 1b9tFk-0003ld-JT
	for qemu-devel@nongnu.org; Mon, 06 Jun 2016 08:03:33 -0400
Received: by mail-vk0-x22d.google.com with SMTP id d64so72135149vkb.0
	for <qemu-devel@nongnu.org>; Mon, 06 Jun 2016 05:03:32 -0700 (PDT)
MIME-Version: 1.0
In-Reply-To: <c0f4e9fb-d194-fecb-c5e7-3634b19a6cf6@redhat.com>
References: <1464694581-31596-1-git-send-email-ppandit@redhat.com>
	<c0f4e9fb-d194-fecb-c5e7-3634b19a6cf6@redhat.com>
From: Peter Maydell <peter.maydell@linaro.org>
Date: Mon, 6 Jun 2016 13:03:12 +0100
Message-ID: <CAFEAcA84MVyAS7Q6QRSt1j9E99d=_Sw-S20hSXk4rhKeG4ykCw@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Subject: Re: [Qemu-devel] [PATCH v2] scsi: esp: check TI buffer index before
 read/write
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel/>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: P J P <ppandit@redhat.com>, Qemu Developers <qemu-devel@nongnu.org>, Li Qiang <liqiang6-s@360.cn>, Huawei PSIRT <PSIRT@huawei.com>, Prasad J Pandit <pjp@fedoraproject.org>

On 6 June 2016 at 13:00, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 31/05/2016 13:36, P J P wrote:
>> +        if ((s->rregs[ESP_RSTAT] & STAT_PIO_MASK) == 0) {
>> +            /* Data out.  */
>> +            qemu_log_mask(LOG_UNIMP, "esp: PIO data read not implemented\n");
>> +            s->rregs[ESP_FIFO] = 0;
>> +            esp_raise_irq(s);
>> +        } else if (s->ti_rptr < s->ti_wptr) {
>>              s->ti_size--;
>> -            if ((s->rregs[ESP_RSTAT] & STAT_PIO_MASK) == 0) {
>> -                /* Data out.  */
>> -                qemu_log_mask(LOG_UNIMP,
>> -                              "esp: PIO data read not implemented\n");
>> -                s->rregs[ESP_FIFO] = 0;
>> -            } else {
>> -                s->rregs[ESP_FIFO] = s->ti_buf[s->ti_rptr++];
>> -            }
>> +            s->rregs[ESP_FIFO] = s->ti_buf[s->ti_rptr++];
>>              esp_raise_irq(s);
>>          }
>>          if (s->ti_size == 0) {
>
> Shouldn't this become a "s->ti_rptr == s->ti_wptr" too?

It would probably be helpful to document what the intended invariant
relating ti_rptr, ti_wptr and ti_size is as well.

thanks
-- PMM