Re: [Qemu-devel] [PATCH for-3.0] slirp: Correct size check in m_inc()

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> On 9 August 2018 at 12:12, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:
> > * Peter Maydell (peter.maydell@linaro.org) wrote:
> >> diff --git a/slirp/mbuf.c b/slirp/mbuf.c
> >> index 0c189e1a7bf..1b7868355a3 100644
> >> --- a/slirp/mbuf.c
> >> +++ b/slirp/mbuf.c
> >> @@ -154,7 +154,7 @@ m_inc(struct mbuf *m, int size)
> >>      int datasize;
> >>
> >>      /* some compilers throw up on gotos.  This one we can fake. */
> >> -    if (m->m_size > size) {
> >> +    if (M_ROOM(m) > size) {
> >>          return;
> >>      }
> >
> > I'm worried about a side effect of this change.
> > A few lines below we have:
> >
> >         datasize = m->m_data - m->m_dat;
> >         m->m_ext = g_malloc(size + datasize);
> >         memcpy(m->m_ext, m->m_dat, m->m_size);
> >         m->m_flags |= M_EXT;
> >
> > Question: What guarantees there's m_size room for that
> > memcpy in the new m_ext?
> 
> It did take me a while to convince myself that that was true
> when I was writing the patch... Here's the ASCII art:
> 
> 
>    |--datasize---->|---m_len------->
>    |----------m_size------------------------------>
>                    |----M_ROOM-------------------->
>                                     |-M_FREEROOM-->
> 
>    ^               ^                               ^
>    m_dat           m_data                          end of buffer
> 
> ("datasize" is a bit misnamed, as it's "size of the leading
> gap between the start of the buffer and the data"; "gapsize"
> would be more helpful.)
> 
> Anyway, we allocate size + datasize, and
> m_size == datasize + M_ROOM. We know that size >= M_ROOM,
> so the allocated buffer must be at least m_size big.

Ah OK, thanks.
(That ascii art could do with being in a comment somewhere!)

Dave

> thanks
> -- PMM
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK