[Spice-devel] [PATCH spice-server] gstreamer: Prevent integer overflow in delay computation

[Frediano Ziglio]

> 
> On Mon, 12 Dec 2016, Frediano Ziglio wrote:
> 
> > The partial expression "MSEC_PER_SEC * size * 8" can overflow if
> > size is 536870 or more. This as the operation is done using
> > 32 bit unsigned integers. Being the size potentially double of
> > a compressed frame size the limit can be easily reached.
> > As get_average_frame_size already return a 64 bit use 64 bit
> > even for the size to avoid the integer overflow.
> > 
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> >  server/gstreamer-encoder.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
> > index e28ab00..988d193 100644
> > --- a/server/gstreamer-encoder.c
> > +++ b/server/gstreamer-encoder.c
> > @@ -511,7 +511,7 @@ static uint32_t get_min_playback_delay(SpiceGstEncoder
> > *encoder)
> >       * an I frame) and an average frame. This also takes into account the
> >       * frames dropped by the encoder bit rate control.
> >       */
> > -    uint32_t size = get_maximum_frame_size(encoder) +
> > get_average_frame_size(encoder);
> > +    uint64_t size = get_maximum_frame_size(encoder) +
> > get_average_frame_size(encoder);
> >      uint32_t send_time = MSEC_PER_SEC * size * 8 / encoder->bit_rate;
> >  
> >      /* Also factor in the network latency with a margin for jitter. */
> > 
> 
> 536870 bytes seems really large for a single frame but being prepared
> for that case makes sense anyway.
> 
> Acked-by: Francois Gouget <fgouget at codeweavers.com>
> 

For mjpeg assuming a 2560x1600 -> 11mb to get about 262kb (536870 / 2)
means a 44:1 compression, not impossible. As somebody is also asking
for bigger resolution this would make more possible the overflow.
The division, which is the most expensive I think is already doing
64 bit / 64 bit. At the and is not so expensive.

By the way, thanks for the review.

Frediano