Re: [qemu-s390x] [PATCH v3 02/25] chardev: Assert IOCanReadHandler can not be negative

[Philippe Mathieu-Daudé]

On 2/20/19 12:13 PM, Philippe Mathieu-Daudé wrote:
> On 2/20/19 11:03 AM, Marc-André Lureau wrote:
>> Hi
>>
>> On Wed, Feb 20, 2019 at 2:03 AM Philippe Mathieu-Daudé
>> <address@hidden> wrote:
>>>
>>> The backend should not return a negative length to read.
>>> We will later change the prototype of IOCanReadHandler to return an
>>> unsigned length. Meanwhile make sure the return length is positive.
>>>
>>> Suggested-by: Paolo Bonzini <address@hidden>
>>> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
>>
>> In such patch, you should do extensive review of existing callbacks,
>> or find a convincing argument that this can't break.
> 
> Argh I missed that.
> 
>> The problem is there are a lot of can_read callbacks, and it's not
>> trivial. The *first* of git-grep is rng_egd_chr_can_read()
>>
>>  57     QSIMPLEQ_FOREACH(req, &s->parent.requests, next) {
>>  58         size += req->size - req->offset;
>>  59     }
>>  60
>>  61     return size;
>>
>> Clearly not obvious if it returns >= 0.
>>
>> Another approach is to look at the caller and the return value
>> handling. If none handle negative values (or would have wrong
>> behaviour with negative values), the assert() is perhaps justified, as
>> it could prevent from doing more harm.
> 
> I'll go and audit all of them.

Actually I already did the work, but it is in the part #2 after this
series, as suggested by Paolo:

https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg02294.html

I'll simply cherry-pick the commit from series #2 before this patch.

Thanks,

Phil.

>>> ---
>>>  chardev/char.c | 5 ++++-
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/chardev/char.c b/chardev/char.c
>>> index f6d61fa5f8..71ecd32b25 100644
>>> --- a/chardev/char.c
>>> +++ b/chardev/char.c
>>> @@ -159,12 +159,15 @@ int qemu_chr_write(Chardev *s, const uint8_t *buf, 
>>> int len, bool write_all)
>>>  int qemu_chr_be_can_write(Chardev *s)
>>>  {
>>>      CharBackend *be = s->be;
>>> +    int receivable_bytes;
>>>
>>>      if (!be || !be->chr_can_read) {
>>>          return 0;
>>>      }
>>>
>>> -    return be->chr_can_read(be->opaque);
>>> +    receivable_bytes = be->chr_can_read(be->opaque);
>>> +    assert(receivable_bytes >= 0);
>>> +    return receivable_bytes;
>>>  }
>>>
>>>  void qemu_chr_be_write_impl(Chardev *s, uint8_t *buf, int len)
>>> --
>>> 2.20.1
>>>