[Spice-devel] [linux-agent v1 2/5] x11-randr: Improve argument check

[Victor Toso]

Hi,

On Tue, Dec 18, 2018 at 12:25:27PM -0500, Frediano Ziglio wrote:
> > 
> > From: Victor Toso <me at victortoso.com>
> > 
> > In the current code, output should never be negative but the check
> > exists after we use the variable as index. Make the check earlier.
> > 
> > Found by coverity:
> >   | check_after_sink: You might be using variable "output" before
> >   | verifying that it is >= 0.
> > 
> > Signed-off-by: Victor Toso <victortoso at redhat.com>
> > ---
> >  src/vdagent/x11-randr.c | 16 +++++++++++-----
> >  1 file changed, 11 insertions(+), 5 deletions(-)
> > 
> > diff --git a/src/vdagent/x11-randr.c b/src/vdagent/x11-randr.c
> > index c8e42c9..03d705f 100644
> > --- a/src/vdagent/x11-randr.c
> > +++ b/src/vdagent/x11-randr.c
> > @@ -347,12 +347,18 @@ static int xrandr_add_and_set(struct vdagent_x11 *x11,
> > int output, int x, int y,
> >      int xid;
> >      Status s;
> >      RROutput outputs[1];
> > -    int old_width  = x11->randr.monitor_sizes[output].width;
> > -    int old_height = x11->randr.monitor_sizes[output].height;
> > +    int old_width;
> > +    int old_height;
> 
> Why not just moving these two definition below?

Because we often declare variables in the begin of scope.

> > -    if (!x11->randr.res || output >= x11->randr.res->noutput || output < 0)
> > {
> > -        syslog(LOG_ERR, "%s: program error: missing RANDR or bad output",
> > -               __FUNCTION__);
> > +    if (output < 0) {
> > +        syslog(LOG_ERR, "%s: program error: bad output", __FUNCTION__);
> > +        return 0;
> > +    }
> > +
> > +    old_width  = x11->randr.monitor_sizes[output].width;
> > +    old_height = x11->randr.monitor_sizes[output].height;
> 
> I suppose skipping the output >= x11->randr.res->noutput now

That's correct, I'll fix this one and resend.

> Coverity should detect an overflow. I assume res and
> monitor_sizes arrays have both size noutput.


> > +    if (!x11->randr.res || output >= x11->randr.res->noutput) {
> > +        syslog(LOG_ERR, "%s: program error: missing RANDR", __FUNCTION__);
> >          return 0;
> >      }
> >      if (x11->set_crtc_config_not_functional) {
> 
> Frediano

Thanks,
Victor
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20181219/5801930e/attachment.sig>