[Spice-devel] [PATCH spice-gtk] Run-time check monitor per display count <= 256

[Marc-André Lureau]

On Wed, Jul 18, 2012 at 9:16 AM, Uri Lublin <uril at redhat.com> wrote:
> Hi Marc-Andre,
>
>
> On 07/17/2012 08:11 PM, Marc-André Lureau wrote:
>>
>> Limit range of monitors, to avoid potential crashes lead by invalid
>> received MonitorConfig values (could be misconfigured or misbehaving
>> guest)
>>
>> This is a a client-side implementation limitation. Eventually, the
>> range could be inscreased (or unlimited == 0) in the future...
>> ---
>>   gtk/channel-display.c |   14 +++++++++++++-
>>   1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/gtk/channel-display.c b/gtk/channel-display.c
>> index d77447a..345d26e 100644
>> --- a/gtk/channel-display.c
>> +++ b/gtk/channel-display.c
>> @@ -275,7 +275,7 @@ static void
>> spice_display_channel_class_init(SpiceDisplayChannelClass *klass)
>>            g_param_spec_uint("monitors-max",
>>                              "Max display monitors",
>>                              "The current maximum number of monitors",
>> -                           1, G_MAXINT16, 1,
>> +                           1, 256, 1,
>
>
> A good idea is to have 256 as a constant.

Done, added MONITORS_CONFIG_MAX to the top of the file.

>
>
>>                              G_PARAM_READABLE |
>>                              G_PARAM_STATIC_STRINGS));
>>
>> @@ -1493,6 +1493,8 @@ static void
>> display_handle_surface_destroy(SpiceChannel *channel, SpiceMsgIn *in
>>       free(surface);
>>   }
>>
>> +#define CLAMP_CHECK(x, low, high)  (((x)>  (high)) ? TRUE : (((x)<
>> (low)) ? TRUE : FALSE))
>> +
>>   /* coroutine context */
>>   static void display_handle_monitors_config(SpiceChannel *channel,
>> SpiceMsgIn *in)
>>   {
>> @@ -1506,6 +1508,16 @@ static void
>> display_handle_monitors_config(SpiceChannel *channel, SpiceMsgIn *in
>>       SPICE_DEBUG("monitors config: n: %d/%d", config->count,
>> config->max_allowed);
>>
>>       c->monitors_max = config->max_allowed;
>> +    if (CLAMP_CHECK(c->monitors_max, 1, 256)) {
>> +        g_warning("MonitorConfig max_allowed is not within permitted
>> range, clamping");
>> +        c->monitors_max = CLAMP(c->monitors_max, 1, 256);
>
>
> I'm not sure it's a good idea to continue and process the message once it
> was
> found to have an invalid number for monitors_max.
> For example if we that value is 1024, CLAMP would make it 256, and the
> loop below would fill 256 entries with possibly bad values.

Well, the remaining monitor config are still valid. In case of having
more >256, what will happen is that the client (virt-viewer for
instance), will only show a couple of them (the one that have a valid
region), and then spice-gtk will limit you to manage only the first
256 monitors, the client won't know there could be even more. It's not
something "bad", it's just a limitation imposed by spice-gtk.

-- 
Marc-André Lureau