| Message ID | 1539919345-10703-25-git-send-email-jasowang@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,V2,01/26] filter-rewriter: Add TCP state machine and fix memory leak in connection_track_table | expand |
Hi Jason, I know that this patch has been already merged to stable, but i have a question: On Fri, Oct 19, 2018 at 11:22:23AM +0800, Jason Wang wrote: > There should not be a reason for passing a packet size greater than > INT_MAX. It's usually a hint of bug somewhere, so ignore packet size > greater than INT_MAX in qemu_deliver_packet_iov() > > CC: qemu-stable@nongnu.org > Reported-by: Daniel Shapira <daniel@twistlock.com> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Jason Wang <jasowang@redhat.com> > --- > net/net.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/net/net.c b/net/net.c > index c66847e..07c194a 100644 > --- a/net/net.c > +++ b/net/net.c > @@ -712,10 +712,15 @@ ssize_t qemu_deliver_packet_iov(NetClientState *sender, > void *opaque) > { > NetClientState *nc = opaque; > + size_t size = iov_size(iov, iovcnt); > int ret; > > + if (size > INT_MAX) { > + return size; Is it okay that the function returns ssize_t (signed), but the type of the size variable is size_t (unsigned)? For now the top level routine checks the return value only for 0, but anyway we can return negative value here instead of positive. What do you think? Regards, Dima. > + } > + > if (nc->link_down) { > - return iov_size(iov, iovcnt); > + return size; > } > > if (nc->receive_disabled) { > -- > 2.5.0 > >
On 2018/11/13 下午11:41, Dima Stepanov wrote: > Hi Jason, > > I know that this patch has been already merged to stable, but i have a > question: > > On Fri, Oct 19, 2018 at 11:22:23AM +0800, Jason Wang wrote: >> There should not be a reason for passing a packet size greater than >> INT_MAX. It's usually a hint of bug somewhere, so ignore packet size >> greater than INT_MAX in qemu_deliver_packet_iov() >> >> CC:qemu-stable@nongnu.org >> Reported-by: Daniel Shapira<daniel@twistlock.com> >> Reviewed-by: Michael S. Tsirkin<mst@redhat.com> >> Signed-off-by: Jason Wang<jasowang@redhat.com> >> --- >> net/net.c | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/net/net.c b/net/net.c >> index c66847e..07c194a 100644 >> --- a/net/net.c >> +++ b/net/net.c >> @@ -712,10 +712,15 @@ ssize_t qemu_deliver_packet_iov(NetClientState *sender, >> void *opaque) >> { >> NetClientState *nc = opaque; >> + size_t size = iov_size(iov, iovcnt); >> int ret; >> >> + if (size > INT_MAX) { >> + return size; > Is it okay that the function returns ssize_t (signed), but the type of the > size variable is size_t (unsigned)? For now the top level routine checks > the return value only for 0, but anyway we can return negative value > here instead of positive. What do you think? > > Regards, Dima. > Any non zero value should be ok here. Actually I think because of the conversion from size_t to ssize_t, caller actually see negative value? Thanks
On Wed, Nov 14, 2018 at 10:59:32AM +0800, Jason Wang wrote: > > On 2018/11/13 下午11:41, Dima Stepanov wrote: > >Hi Jason, > > > >I know that this patch has been already merged to stable, but i have a > >question: > > > >On Fri, Oct 19, 2018 at 11:22:23AM +0800, Jason Wang wrote: > >>There should not be a reason for passing a packet size greater than > >>INT_MAX. It's usually a hint of bug somewhere, so ignore packet size > >>greater than INT_MAX in qemu_deliver_packet_iov() > >> > >>CC:qemu-stable@nongnu.org > >>Reported-by: Daniel Shapira<daniel@twistlock.com> > >>Reviewed-by: Michael S. Tsirkin<mst@redhat.com> > >>Signed-off-by: Jason Wang<jasowang@redhat.com> > >>--- > >> net/net.c | 7 ++++++- > >> 1 file changed, 6 insertions(+), 1 deletion(-) > >> > >>diff --git a/net/net.c b/net/net.c > >>index c66847e..07c194a 100644 > >>--- a/net/net.c > >>+++ b/net/net.c > >>@@ -712,10 +712,15 @@ ssize_t qemu_deliver_packet_iov(NetClientState *sender, > >> void *opaque) > >> { > >> NetClientState *nc = opaque; > >>+ size_t size = iov_size(iov, iovcnt); > >> int ret; > >>+ if (size > INT_MAX) { > >>+ return size; > >Is it okay that the function returns ssize_t (signed), but the type of the > >size variable is size_t (unsigned)? For now the top level routine checks > >the return value only for 0, but anyway we can return negative value > >here instead of positive. What do you think? > > > >Regards, Dima. > > > > Any non zero value should be ok here. Actually I think because of the > conversion from size_t to ssize_t, caller actually see negative value? I believe it depends. If long (ssize_t and size_t type) is 8 bytes, then the routine can sometimes return positive values and sometimes negative. I fully agree that in the current case any non zero value should be okay. I just wanted to point on the inconsistency in types and as a result a return value. Dima. > > Thanks >
On 2018/11/15 上午12:23, Dima Stepanov wrote: > On Wed, Nov 14, 2018 at 10:59:32AM +0800, Jason Wang wrote: >> On 2018/11/13 下午11:41, Dima Stepanov wrote: >>> Hi Jason, >>> >>> I know that this patch has been already merged to stable, but i have a >>> question: >>> >>> On Fri, Oct 19, 2018 at 11:22:23AM +0800, Jason Wang wrote: >>>> There should not be a reason for passing a packet size greater than >>>> INT_MAX. It's usually a hint of bug somewhere, so ignore packet size >>>> greater than INT_MAX in qemu_deliver_packet_iov() >>>> >>>> CC:qemu-stable@nongnu.org >>>> Reported-by: Daniel Shapira<daniel@twistlock.com> >>>> Reviewed-by: Michael S. Tsirkin<mst@redhat.com> >>>> Signed-off-by: Jason Wang<jasowang@redhat.com> >>>> --- >>>> net/net.c | 7 ++++++- >>>> 1 file changed, 6 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/net/net.c b/net/net.c >>>> index c66847e..07c194a 100644 >>>> --- a/net/net.c >>>> +++ b/net/net.c >>>> @@ -712,10 +712,15 @@ ssize_t qemu_deliver_packet_iov(NetClientState *sender, >>>> void *opaque) >>>> { >>>> NetClientState *nc = opaque; >>>> + size_t size = iov_size(iov, iovcnt); >>>> int ret; >>>> + if (size > INT_MAX) { >>>> + return size; >>> Is it okay that the function returns ssize_t (signed), but the type of the >>> size variable is size_t (unsigned)? For now the top level routine checks >>> the return value only for 0, but anyway we can return negative value >>> here instead of positive. What do you think? >>> >>> Regards, Dima. >>> >> Any non zero value should be ok here. Actually I think because of the >> conversion from size_t to ssize_t, caller actually see negative value? > I believe it depends. If long (ssize_t and size_t type) is 8 bytes, then > the routine can sometimes return positive values and sometimes negative. > I fully agree that in the current case any non zero value should be > okay. I just wanted to point on the inconsistency in types and as a > result a return value. I see, want to post a patch for this? Thanks > Dima. >> Thanks >>
On Thu, Nov 15, 2018 at 10:47:04AM +0800, Jason Wang wrote: > > On 2018/11/15 上午12:23, Dima Stepanov wrote: > >On Wed, Nov 14, 2018 at 10:59:32AM +0800, Jason Wang wrote: > >>On 2018/11/13 下午11:41, Dima Stepanov wrote: > >>>Hi Jason, > >>> > >>>I know that this patch has been already merged to stable, but i have a > >>>question: > >>> > >>>On Fri, Oct 19, 2018 at 11:22:23AM +0800, Jason Wang wrote: > >>>>There should not be a reason for passing a packet size greater than > >>>>INT_MAX. It's usually a hint of bug somewhere, so ignore packet size > >>>>greater than INT_MAX in qemu_deliver_packet_iov() > >>>> > >>>>CC:qemu-stable@nongnu.org > >>>>Reported-by: Daniel Shapira<daniel@twistlock.com> > >>>>Reviewed-by: Michael S. Tsirkin<mst@redhat.com> > >>>>Signed-off-by: Jason Wang<jasowang@redhat.com> > >>>>--- > >>>> net/net.c | 7 ++++++- > >>>> 1 file changed, 6 insertions(+), 1 deletion(-) > >>>> > >>>>diff --git a/net/net.c b/net/net.c > >>>>index c66847e..07c194a 100644 > >>>>--- a/net/net.c > >>>>+++ b/net/net.c > >>>>@@ -712,10 +712,15 @@ ssize_t qemu_deliver_packet_iov(NetClientState *sender, > >>>> void *opaque) > >>>> { > >>>> NetClientState *nc = opaque; > >>>>+ size_t size = iov_size(iov, iovcnt); > >>>> int ret; > >>>>+ if (size > INT_MAX) { > >>>>+ return size; > >>>Is it okay that the function returns ssize_t (signed), but the type of the > >>>size variable is size_t (unsigned)? For now the top level routine checks > >>>the return value only for 0, but anyway we can return negative value > >>>here instead of positive. What do you think? > >>> > >>>Regards, Dima. > >>> > >>Any non zero value should be ok here. Actually I think because of the > >>conversion from size_t to ssize_t, caller actually see negative value? > >I believe it depends. If long (ssize_t and size_t type) is 8 bytes, then > >the routine can sometimes return positive values and sometimes negative. > >I fully agree that in the current case any non zero value should be > >okay. I just wanted to point on the inconsistency in types and as a > >result a return value. > > > I see, want to post a patch for this? > > Thanks Yes, will take a look into it and prepare a patch. Thanks, Dima. > > > >Dima. > >>Thanks > >>
diff --git a/net/net.c b/net/net.c index c66847e..07c194a 100644 --- a/net/net.c +++ b/net/net.c @@ -712,10 +712,15 @@ ssize_t qemu_deliver_packet_iov(NetClientState *sender, void *opaque) { NetClientState *nc = opaque; + size_t size = iov_size(iov, iovcnt); int ret; + if (size > INT_MAX) { + return size; + } + if (nc->link_down) { - return iov_size(iov, iovcnt); + return size; } if (nc->receive_disabled) {