[virt-tools-list] [PATCH virt-viewer 15/19] Hook up handling of Monitors

[Marc-André Lureau]

Hi

On Tue, Jul 17, 2012 at 5:41 PM, Christophe Fergeau <cfergeau at redhat.com> wrote:
>> G_INT_MAX / sizeof(void *) ? where did you get the / sizeof(void*) from?
>
> You are using a GPtrArray, so my understanding is that the memory allocated
> for it is num_elements * sizeof (void*). If num_elements > G_INT_MAX /
> sizeof(void*), a quick reading suggests this will cause an overflow in
> the amount of memory to allocate causing the allocated array to be much
> smaller than what is expected. Maybe that is not an issue, but this is

It looks like GPtrArry uses guint type, but set_size() is gint.. If
there is an overflow in array length computation, it should be handled
by glib perhaps.

> worrying. My point was that you are saying that allocating too much memory
> will only cause swapping and the OOM to trigger, and I wanted to point out
> that in this case, the problem could not be easily simplified to "too much
> memory allocated, but no buffer overflow".

this is not yet a buffer overflow proof, but you are getting closer perhaps.

>>
>> Eh ok, so you want to check if max_monitors > G_INT_MAX.. :) a very
>> unlikely condition that will crash the client anyway. But fine by me.
>
> Not really, just trying to understand what the extent of the problem is,
> and quite worried that you are basically saying this is not an issue at all
> and that all is fine.

Until we can really point clearly the issue.


>> >> There is nothing wrong in OOM if the server tells us we have 1024
>> >> maximum monitors on the guest for example,
>> >
>> > Is this max value coming from the server, or is it the guest qxl driver
>> > telling the server about how many monitors it supports?
>>
>> It's configured at xorg level, with NumHeads option, defaulting to 4.
>
> So what we are trusting here is an arbitrary value provided by the guest
> system?

Yes, no further checks after that afaict. So a misconfigured guest
could trigger this error perhaps.


-- 
Marc-André Lureau