diff mbox

[ovs-dev,v1] bridge: Fix qos_unixctl_show bug.

Message ID 1457478631-17351-1-git-send-email-ian.stokes@intel.com
State Accepted
Headers show

Commit Message

Stokes, Ian March 8, 2016, 11:10 p.m. UTC 
netdev_get_qos returns a value to indicate if an error has occurred while
attempting to query the QoS configuration of an interface. If an error does
occur the pointer argument passed to it will be set to null before returning.
Currently the vswitch will segfault if this occurs as qos_unixctl_show will
attempt to access the pointer directly after it calls netdev_get_qos.

Avoid this by adding a check for the return value and flagging an appropriate
error message to appctl and the vswitch log.

Signed-off-by: Ian Stokes <ian.stokes@intel.com>
---
 vswitchd/bridge.c |   40 ++++++++++++++++++++++++----------------
 1 files changed, 24 insertions(+), 16 deletions(-)

Comments

Ben Pfaff March 22, 2016, 5:58 p.m. UTC | #1 
On Tue, Mar 08, 2016 at 11:10:31PM +0000, Ian Stokes wrote:
> netdev_get_qos returns a value to indicate if an error has occurred while
> attempting to query the QoS configuration of an interface. If an error does
> occur the pointer argument passed to it will be set to null before returning.
> Currently the vswitch will segfault if this occurs as qos_unixctl_show will
> attempt to access the pointer directly after it calls netdev_get_qos.
> 
> Avoid this by adding a check for the return value and flagging an appropriate
> error message to appctl and the vswitch log.
> 
> Signed-off-by: Ian Stokes <ian.stokes@intel.com>

Thanks!  I applied this to master, branch-2.5, branch-2.4, and
branch-2.3.
diff mbox

Patch

diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index 25a0663..1133bcd 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -3130,6 +3130,7 @@  qos_unixctl_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
     struct iface *iface;
     const char *type;
     struct smap_node *node;
+    int error;
 
     iface = iface_find(argv[1]);
     if (!iface) {
@@ -3137,29 +3138,36 @@  qos_unixctl_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
         return;
     }
 
-    netdev_get_qos(iface->netdev, &type, &smap);
+    error = netdev_get_qos(iface->netdev, &type, &smap);
+    if (!error) {
+        if (*type != '\0') {
+            struct netdev_queue_dump dump;
+            struct smap details;
+            unsigned int queue_id;
 
-    if (*type != '\0') {
-        struct netdev_queue_dump dump;
-        struct smap details;
-        unsigned int queue_id;
+            ds_put_format(&ds, "QoS: %s %s\n", iface->name, type);
 
-        ds_put_format(&ds, "QoS: %s %s\n", iface->name, type);
+            SMAP_FOR_EACH (node, &smap) {
+                ds_put_format(&ds, "%s: %s\n", node->key, node->value);
+            }
 
-        SMAP_FOR_EACH (node, &smap) {
-            ds_put_format(&ds, "%s: %s\n", node->key, node->value);
-        }
+            smap_init(&details);
+            NETDEV_QUEUE_FOR_EACH (&queue_id, &details, &dump, iface->netdev) {
+                qos_unixctl_show_queue(queue_id, &details, iface, &ds);
+            }
+            smap_destroy(&details);
 
-        smap_init(&details);
-        NETDEV_QUEUE_FOR_EACH (&queue_id, &details, &dump, iface->netdev) {
-            qos_unixctl_show_queue(queue_id, &details, iface, &ds);
+            unixctl_command_reply(conn, ds_cstr(&ds));
+        } else {
+            ds_put_format(&ds, "QoS not configured on %s\n", iface->name);
+            unixctl_command_reply_error(conn, ds_cstr(&ds));
         }
-        smap_destroy(&details);
-
-        unixctl_command_reply(conn, ds_cstr(&ds));
     } else {
-        ds_put_format(&ds, "QoS not configured on %s\n", iface->name);
+        ds_put_format(&ds, "Could not retrieve QoS configuration details on %s"
+                           " due to error\n", iface->name);
         unixctl_command_reply_error(conn, ds_cstr(&ds));
+        VLOG_ERR("interface %s could not get QoS configuration, failed with"
+                 " errno %d", iface->name, error);
     }
 
     smap_destroy(&smap);