| Message ID | 20190220010232.18731-3-philmd@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | chardev: Convert qemu_chr_write() to take a size_t argument | expand |
Hi On Wed, Feb 20, 2019 at 2:03 AM Philippe Mathieu-Daudé <philmd@redhat.com> 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 <pbonzini@redhat.com> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> In such patch, you should do extensive review of existing callbacks, or find a convincing argument that this can't break. 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. > --- > 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 >
On 2/20/19 11:03 AM, Marc-André Lureau wrote: > Hi > > On Wed, Feb 20, 2019 at 2:03 AM Philippe Mathieu-Daudé > <philmd@redhat.com> 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 <pbonzini@redhat.com> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > 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. Thanks for the review! 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 >>
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é >> <philmd@redhat.com> 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 <pbonzini@redhat.com> >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> >> 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 >>>
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)
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 <pbonzini@redhat.com> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- chardev/char.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)