[Spice-devel,win-vdagent] vdagent: protect against NULL entry in _displays

Submitted by Uri Lublin on April 30, 2013, 10:10 a.m.

Details

Message ID 67e4136f8f7cd5757613514b172ecc3c7f8cb250.1367314869.git.uril@redhat.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Uri Lublin April 30, 2013, 10:10 a.m.
rhbz#958051

It may be that a _displays entry will be NULL.
I encountered it when running with multiple QXL devices, for one of
which the driver failed to load since it was "out of resources"

Iterations over _displays should handle that case.
I found three such iterations, and fixed them in this patch
---
 vdagent/desktop_layout.cpp |    3 +++
 vdagent/vdagent.cpp        |    9 ++++++---
 2 files changed, 9 insertions(+), 3 deletions(-)

Patch hide | download patch | download mbox

diff --git a/vdagent/desktop_layout.cpp b/vdagent/desktop_layout.cpp
index c474edb..7dadc9a 100644
--- a/vdagent/desktop_layout.cpp
+++ b/vdagent/desktop_layout.cpp
@@ -155,6 +155,9 @@  void DesktopLayout::normalize_displays_pos()
 
     for (iter = _displays.begin(); iter != _displays.end(); iter++) {
         mode = *iter;
+        if (!mode) {
+            continue;
+        }
         if (mode->_attached) {
             min_x = min(min_x, mode->_pos_x);
             min_y = min(min_y, mode->_pos_y);
diff --git a/vdagent/vdagent.cpp b/vdagent/vdagent.cpp
index dde967c..3423aa1 100644
--- a/vdagent/vdagent.cpp
+++ b/vdagent/vdagent.cpp
@@ -613,7 +613,9 @@  bool VDAgent::handle_mon_config(VDAgentMonitorsConfig* mon_config, uint32_t port
     display_count = _desktop_layout->get_display_count();
     for (uint32_t i = 0; i < display_count; i++) {
         DisplayMode* mode = _desktop_layout->get_display(i);
-        ASSERT(mode);
+        if (!mode) {
+            continue;
+        }
         if (i >= mon_config->num_of_monitors) {
             vd_printf("%d. detached", i);
             mode->set_attached(false);
@@ -738,8 +740,9 @@  void VDAgent::set_display_depth(uint32_t depth)
     // setting depth for all the monitors, including unattached ones
     for (uint32_t i = 0; i < display_count; i++) {
         DisplayMode* mode = _desktop_layout->get_display(i);
-        ASSERT(mode);
-        mode->set_depth(depth);
+        if (mode) {
+            mode->set_depth(depth);
+        }
     }
 
     if (display_count) {

Comments

ack

On 04/30/2013 01:10 PM, Uri Lublin wrote:
> rhbz#958051
>
> It may be that a _displays entry will be NULL.
> I encountered it when running with multiple QXL devices, for one of
> which the driver failed to load since it was "out of resources"
>
> Iterations over _displays should handle that case.
> I found three such iterations, and fixed them in this patch
> ---
>   vdagent/desktop_layout.cpp |    3 +++
>   vdagent/vdagent.cpp        |    9 ++++++---
>   2 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/vdagent/desktop_layout.cpp b/vdagent/desktop_layout.cpp
> index c474edb..7dadc9a 100644
> --- a/vdagent/desktop_layout.cpp
> +++ b/vdagent/desktop_layout.cpp
> @@ -155,6 +155,9 @@ void DesktopLayout::normalize_displays_pos()
>   
>       for (iter = _displays.begin(); iter != _displays.end(); iter++) {
>           mode = *iter;
> +        if (!mode) {
> +            continue;
> +        }
>           if (mode->_attached) {
>               min_x = min(min_x, mode->_pos_x);
>               min_y = min(min_y, mode->_pos_y);
> diff --git a/vdagent/vdagent.cpp b/vdagent/vdagent.cpp
> index dde967c..3423aa1 100644
> --- a/vdagent/vdagent.cpp
> +++ b/vdagent/vdagent.cpp
> @@ -613,7 +613,9 @@ bool VDAgent::handle_mon_config(VDAgentMonitorsConfig* mon_config, uint32_t port
>       display_count = _desktop_layout->get_display_count();
>       for (uint32_t i = 0; i < display_count; i++) {
>           DisplayMode* mode = _desktop_layout->get_display(i);
> -        ASSERT(mode);
> +        if (!mode) {
> +            continue;
> +        }
>           if (i >= mon_config->num_of_monitors) {
>               vd_printf("%d. detached", i);
>               mode->set_attached(false);
> @@ -738,8 +740,9 @@ void VDAgent::set_display_depth(uint32_t depth)
>       // setting depth for all the monitors, including unattached ones
>       for (uint32_t i = 0; i < display_count; i++) {
>           DisplayMode* mode = _desktop_layout->get_display(i);
> -        ASSERT(mode);
> -        mode->set_depth(depth);
> +        if (mode) {
> +            mode->set_depth(depth);
> +        }
>       }
>   
>       if (display_count) {
Hi

On Tue, Apr 30, 2013 at 12:10 PM, Uri Lublin <uril@redhat.com> wrote:

>      if (display_count) {


There is at least one more use you forgot, and that caused still a crash

 if (min_x || min_y) {
         for (iter = _displays.begin(); iter != _displays.end(); iter++) {
+if (!*iter) {
+   continue;
+}
So when this happens (even if the agent is fixed to not crash), then we get
randomly loaded drivers assigned to channels

A guest monitor 3 might end up on display channel 4 if for example, driver
failed to load for card associated to display channel 3..

This means that virt-viewer will show a "waiting for display" message for
the disabled monitor, and it will enable monitors on unexpected channels
(turning on 3 will enable 4...)

I wonder if the VM shouldn't just refuse to start if this ressource problem
occurs.

Is it a VRAM resource limit?


On Fri, May 3, 2013 at 12:57 AM, Marc-André Lureau <
marcandre.lureau@gmail.com> wrote:

> Hi
>
> On Tue, Apr 30, 2013 at 12:10 PM, Uri Lublin <uril@redhat.com> wrote:
>
>>      if (display_count) {
>
>
> There is at least one more use you forgot, and that caused still a crash
>
>  if (min_x || min_y) {
>
>          for (iter = _displays.begin(); iter != _displays.end(); iter++) {
> +if (!*iter) {
> +   continue;
> +}
>
>
> --
> Marc-André Lureau
>
On 05/03/2013 01:57 AM, Marc-André Lureau wrote:
> Hi
>
> On Tue, Apr 30, 2013 at 12:10 PM, Uri Lublin <uril@redhat.com 
> <mailto:uril@redhat.com>> wrote:
>
>          if (display_count) {
>
>
> There is at least one more use you forgot, and that caused still a crash
>
>  if (min_x || min_y) {
>          for (iter = _displays.begin(); iter != _displays.end(); iter++) {
> +if (!*iter) {
> +   continue;
> +}
>

You are right, thanks.

I'll send a V2.
On 05/03/2013 02:03 AM, Marc-André Lureau wrote:
> So when this happens (even if the agent is fixed to not crash), then 
> we get randomly loaded drivers assigned to channels
>
> A guest monitor 3 might end up on display channel 4 if for example, 
> driver failed to load for card associated to display channel 3..

Right, the mapping between physical and virtual displays is random.
Maybe we can show that map to users, and even let him to change it 
somehow, according
to their liking. That's a new feature, regardless of this patch.

>
> This means that virt-viewer will show a "waiting for display" message 
> for the disabled monitor, and it will enable monitors on unexpected 
> channels (turning on 3 will enable 4...)

That's a indeed confusing, but the user can figure out the right mapping 
and not use the disabled monitor.

>
> I wonder if the VM shouldn't just refuse to start if this ressource 
> problem occurs.

Maybe.
But maybe using 3 monitors is better than not being able to start.

>
> Is it a VRAM resource limit?
>
It has to do with the VRAM (and RAM) size.
I think it's something to do with the bios (memory space is limited).

Thanks for reviewing,
     Uri.