Bump for review.

On 08/05/2016 02:33 PM, Pavel Vomacka wrote:


On 08/01/2016 05:53 PM, Petr Vobornik wrote:
On 07/29/2016 03:25 PM, Alexander Bokovoy wrote:
On Fri, 29 Jul 2016, Pavel Vomacka wrote:
Hello,

please review attached patches which fixes errors from Coverity.

--
Pavel^3 Vomacka

From 0391289b3f6844897e2a9f3ae549bd4c33233ffc Mon Sep 17 00:00:00 2001
From: Pavel Vomacka <pvoma...@redhat.com>
Date: Mon, 25 Jul 2016 10:36:47 +0200
Subject: [PATCH 01/13] Coverity - null pointer exception

Variable 'option' can be null and there will be error of reading
property of null.
---
install/ui/src/freeipa/widget.js | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/install/ui/src/freeipa/widget.js
b/install/ui/src/freeipa/widget.js
index
9151ebac9438e9e674f81bfb1ccfe7a63872b1ae..cfdf5d4750951e4549c16a2b9b9c355f61e90c39
100644
--- a/install/ui/src/freeipa/widget.js
+++ b/install/ui/src/freeipa/widget.js
@@ -2249,7 +2249,7 @@ IPA.option_widget_base = function(spec, that) {
                 var child_values = [];
                 var option = that.get_option(value);

-                if (option.widget) {
+                if (option && option.widget) {
                     child_values = option.widget.save();
                     values.push.apply(values, child_values);
                 }
--
2.5.5

ACK
ACK

From 6df8e608232e25daa9aefe4fccbdeca4dbaf1998 Mon Sep 17 00:00:00 2001
From: Pavel Vomacka <pvoma...@redhat.com>
Date: Mon, 25 Jul 2016 10:43:00 +0200
Subject: [PATCH 02/13] Coverity - null pointer exception

Variable 'row' could be null in some cases. And set css to variable
which is pointing to null
causes error. Therefore there is new check.
---
install/ui/src/freeipa/widget.js | 2 ++
1 file changed, 2 insertions(+)

diff --git a/install/ui/src/freeipa/widget.js
b/install/ui/src/freeipa/widget.js
index
cfdf5d4750951e4549c16a2b9b9c355f61e90c39..5844436abf090f12d5a9d65efe7a1aaee14097e2
100644
--- a/install/ui/src/freeipa/widget.js
+++ b/install/ui/src/freeipa/widget.js
@@ -5766,6 +5766,8 @@ exp.fluid_layout = IPA.fluid_layout =
function(spec) {
     that.on_visible_change = function(event) {

         var row = that._get_row(event);
+        if (!row) return;
+
         if (event.visible) {
             row.css('display', '');
         } else {
--
2.5.5

ACK

ACK


From 6f2ddc9e1c5323a640bdf744d2da00bfee7ab766 Mon Sep 17 00:00:00 2001
From: Pavel Vomacka <pvoma...@redhat.com>
Date: Mon, 25 Jul 2016 13:48:16 +0200
Subject: [PATCH 03/13] Coverity - not initialized variable

The variable hasn't been initialized, now it is set to null by default.
---
install/ui/src/freeipa/widget.js | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/install/ui/src/freeipa/widget.js
b/install/ui/src/freeipa/widget.js
index
5844436abf090f12d5a9d65efe7a1aaee14097e2..43804c5ea524ca741017d02f6e12ccf60d50b5df
100644
--- a/install/ui/src/freeipa/widget.js
+++ b/install/ui/src/freeipa/widget.js
@@ -1047,7 +1047,7 @@ IPA.multivalued_widget = function(spec) {

     that.child_spec = spec.child_spec;
     that.size = spec.size || 30;
-    that.undo_control;
+    that.undo_control = null;
     that.initialized = true;
     that.updating = false;

--
2.5.5

ACK
ACK


From b9ddd32ec45aadae5a79e372c3e1b70990071e60 Mon Sep 17 00:00:00 2001
From: Pavel Vomacka <pvoma...@redhat.com>
Date: Mon, 25 Jul 2016 14:42:50 +0200
Subject: [PATCH 04/13] Coverity - identical code for different branches

In both cases when the condition is true or false ut is set the same
value.
Changed to assign the value directly.
---
install/ui/src/freeipa/topology_graph.js | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/install/ui/src/freeipa/topology_graph.js
b/install/ui/src/freeipa/topology_graph.js
index
ce2ebeaff611987ae27f2655b5da80bdcd1b4f8a..712d38fbe67e87ffa773e0a3a1f8937e9595c9a6
100644
--- a/install/ui/src/freeipa/topology_graph.js
+++ b/install/ui/src/freeipa/topology_graph.js
@@ -325,8 +325,8 @@ topology_graph.TopoGraph = declare([Evented], {
                 off = dir ? -1 : 1, // determines shift direction of
curve
                 ns = 5, // shift on normal vector
                 s = target_count > 1 ? 1 : 0, // shift from center?
-                spad = d.left ? 18 : 18, // source padding
-                tpad = d.right ? 18 : 18, // target padding
+                spad = d.left = 18, // source padding
+                tpad = d.right = 18, // target padding
sourceX = d.source.x + (spad * ux) + off * nx * ns * s, sourceY = d.source.y + (spad * uy) + off * ny * ns * s, targetX = d.target.x - (tpad * ux) + off * nx * ns * s,
--
2.5.5

ACK
NACK

following lines are not equivalent
    spad = d.left ? 18 : 18
    spad = d.left = 18

same with tpad
Fixed
From f1f2b55247d6c7f41f8053f372a47945c93fc8a4 Mon Sep 17 00:00:00 2001
From: Pavel Vomacka <pvoma...@redhat.com>
Date: Mon, 25 Jul 2016 14:52:15 +0200
Subject: [PATCH 05/13] Coverity - Accesing attribute of null

There is a possibility that widget is null and then there could be an
error.
Therefore there is new check of widget variable.
---
install/ui/src/freeipa/widgets/APIBrowserWidget.js | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/install/ui/src/freeipa/widgets/APIBrowserWidget.js
b/install/ui/src/freeipa/widgets/APIBrowserWidget.js
index
1a3726190d4a5d628a8f7c2b564c4c9f6e7cea1f..50c2989fcc126585787df61cdd19493632ed37b9
100644
--- a/install/ui/src/freeipa/widgets/APIBrowserWidget.js
+++ b/install/ui/src/freeipa/widgets/APIBrowserWidget.js
@@ -252,7 +252,7 @@ widgets.APIBrowserWidget = declare([Stateful,
Evented], {
         }

         // switch widget
-        if (!widget.el) widget.render();
+        if (widget && !widget.el) widget.render();
         if (this.current_details_w !== widget) {
             this.details_el.empty();
             this.details_el.append(widget.el);
--
2.5.5

ACK
ACK

From 1476b5ed3ab5c4ec55f3ed20ad07a5b88cfd45f2 Mon Sep 17 00:00:00 2001
From: Pavel Vomacka <pvoma...@redhat.com>
Date: Mon, 25 Jul 2016 16:47:22 +0200
Subject: [PATCH 06/13] Coverity - removed dead code

There cannot be string value because of previous checks.
---
install/ui/src/freeipa/dns.js | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/install/ui/src/freeipa/dns.js
b/install/ui/src/freeipa/dns.js
index
2d424aeae8ef735d02426a0f08b6261ec2f04c19..822c0b3cedb3988563c0a1f83862f56e95eed21b
100644
--- a/install/ui/src/freeipa/dns.js
+++ b/install/ui/src/freeipa/dns.js
@@ -1509,14 +1509,10 @@ IPA.dns.record_prepare_editor_for_type =
function(type, fields, widgets, update)

         //create editor widget
         var widget = {};
-        if (typeof attribute === 'string') {
-            widget.name = attribute;
-        } else {
-            widget.name = attribute.name;
-            set_defined(attribute.$type, widget, '$type');
-            set_defined(attribute.options, widget, 'options');
-            copy_obj(widget, attribute.widget_opt);
-        }
+        widget.name = attribute.name;
+        set_defined(attribute.$type, widget, '$type');
+        set_defined(attribute.options, widget, 'options');
+        copy_obj(widget, attribute.widget_opt);
         section.widgets.push(widget);
     }
};
--
2.5.5

ACK
ACK


From b1dd66f3b08889b51430d9176035366cb055324e Mon Sep 17 00:00:00 2001
From: Pavel Vomacka <pvoma...@redhat.com>
Date: Mon, 25 Jul 2016 17:44:56 +0200
Subject: [PATCH 07/13] Coverity - true branch can't be executed

The 'data' variable is always false because of previous condition.
Therefore there is direct assignment.
---
install/ui/src/freeipa/rpc.js | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/install/ui/src/freeipa/rpc.js
b/install/ui/src/freeipa/rpc.js
index
a185585f4176658e299e7e92434522c936cc36b4..88aaf6ede72ea69495c369dd74c657d0419a3605
100644
--- a/install/ui/src/freeipa/rpc.js
+++ b/install/ui/src/freeipa/rpc.js
@@ -372,7 +372,7 @@ rpc.command = function(spec) {
                 error_handler.call(this, xhr, text_status, /*
error_thrown */ {
                     name: text.get('@i18n:errors.http_error', 'HTTP
Error')+' '+xhr.status,
                     url: this.url,
-                    message: data ? xhr.statusText :
text.get('@i18n:errors.no_response', 'No response')
+                    message: text.get('@i18n:errors.no_response', 'No
response')
                 });

             } else if (IPA.version && data.version && IPA.version !==
data.version) {
--
2.5.5

ACK

ACK - patch fixes the issue.

But I wonder if it should be rather:
   message: xhr ? xhr.statusText : text.get('@i18n:errors.no_response',
'No response')

don't remember.
That's true, fixed.

From 463f24936469d87890b666dfd7edabbe90541491 Mon Sep 17 00:00:00 2001
From: Pavel Vomacka <pvoma...@redhat.com>
Date: Mon, 25 Jul 2016 17:49:50 +0200
Subject: [PATCH 08/13] Coverity - true branch can't be executed

The 'result' variable is always false because of previous condition.
Therefore there is direct assignment.
---
install/ui/src/freeipa/rpc.js | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/install/ui/src/freeipa/rpc.js
b/install/ui/src/freeipa/rpc.js
index
88aaf6ede72ea69495c369dd74c657d0419a3605..30a5366787974b2d127114f7683d0589ed332f5a
100644
--- a/install/ui/src/freeipa/rpc.js
+++ b/install/ui/src/freeipa/rpc.js
@@ -628,7 +628,7 @@ rpc.batch_command = function(spec) {

             if (!result) {
                 name = text.get('@i18n:errors.internal_error',
'Internal Error')+' '+xhr.status;
-                message = result ? xhr.statusText :
text.get('@i18n:errors.internal_error', 'Internal Error');
+                message = text.get('@i18n:errors.internal_error',
'Internal Error');

                 that.errors.add(command, name, message, text_status);

--
2.5.5

ACK
same as previous
Fixed as well.
From c0ba1c141b6191e2a7ef33bc9eaaad5c970f9d0e Mon Sep 17 00:00:00 2001
From: Pavel Vomacka <pvoma...@redhat.com>
Date: Mon, 25 Jul 2016 18:25:36 +0200
Subject: [PATCH 09/13] Coverity - null pointer dereference

The 'obj' variable could be null, so there could be error when it is
used.
A new check that 'obj' is not false is added.
---
install/ui/src/freeipa/widgets/browser_widgets.js | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/install/ui/src/freeipa/widgets/browser_widgets.js
b/install/ui/src/freeipa/widgets/browser_widgets.js
index
57ad2bd984ea35f03b302b59fc1d014def162bd8..91bb850a638fd6f16f207b1111d126fbb4fe2dd8
100644
--- a/install/ui/src/freeipa/widgets/browser_widgets.js
+++ b/install/ui/src/freeipa/widgets/browser_widgets.js
@@ -427,11 +427,11 @@ widgets.browser_widgets.CommandDetailWidget =
declare([base], {
                 if (i>0) {
                     out_params_cnt.append(', ');
                 }
-                if (!param) {
-                    out_params_cnt.append(param_name);
-                } else {
+                if (param && obj) {
                     var link = this.render_param_link(obj.name,
param_name);
                     out_params_cnt.append(link);
+                } else {
+                    out_params_cnt.append(param_name);
                 }
             }
             out_params_cnt.appendTo(this.el);
--
2.5.5

ACK
ACK

From a9f7ecf5833db379fe9731184aa4f7aef8845995 Mon Sep 17 00:00:00 2001
From: Pavel Vomacka <pvoma...@redhat.com>
Date: Tue, 26 Jul 2016 09:48:32 +0200
Subject: [PATCH 10/13] Coverity - iterating over variable which could
be null

Change condition to check also variable which could be null.
---
install/ui/src/freeipa/widgets/APIBrowserWidget.js | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/install/ui/src/freeipa/widgets/APIBrowserWidget.js
b/install/ui/src/freeipa/widgets/APIBrowserWidget.js
index
50c2989fcc126585787df61cdd19493632ed37b9..18773536d3587cdeb9e5fecedcc5e42c05bfe120
100644
--- a/install/ui/src/freeipa/widgets/APIBrowserWidget.js
+++ b/install/ui/src/freeipa/widgets/APIBrowserWidget.js
@@ -135,7 +135,7 @@ widgets.APIBrowserWidget = declare([Stateful,
Evented], {
             groups = this._get_params(parts[0]);
         }

-        if (filter) {
+        if (filter && groups) {
             filter = filter.toLowerCase();
             var new_groups = [];
             for (var i=0,l=groups.length; i<l; i++) {
@@ -153,10 +153,10 @@ widgets.APIBrowserWidget = declare([Stateful,
Evented], {
                     new_groups.push(groups[i]);
                 }
             }
-            return new_groups;
-        } else {
-            return groups;
+            groups = new_groups;
         }
+
+        return groups;
     },

     /**
--
2.5.5

ACK
ACK


From 3d63ca1d5cb7a7b84cf20c26d4b1ea5b657c44c4 Mon Sep 17 00:00:00 2001
From: Pavel Vomacka <pvoma...@redhat.com>
Date: Tue, 26 Jul 2016 12:03:28 +0200
Subject: [PATCH 11/13] Coverity - opens dialog which might not be created

Check whether dialog object is created before opening it.
---
install/ui/src/freeipa/search.js | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/install/ui/src/freeipa/search.js
b/install/ui/src/freeipa/search.js
index
25f21e70db170daf0d45a6862ee9adb528ad03bc..fee1bc7523d6afdb3e2b23db2833a415febb85ec
100644
--- a/install/ui/src/freeipa/search.js
+++ b/install/ui/src/freeipa/search.js
@@ -221,7 +221,7 @@ IPA.search_facet = function(spec, no_init) {
     that.show_remove_dialog = function() {

         var dialog = that.create_remove_dialog();
-        dialog.open();
+        if (dialog) dialog.open();
     };

     that.find = function() {
--
2.5.5

ACK

ACK but question is whether we should laso log to console that dialog is
not defined because it just hides an issue which may be harder to debug.

It's a good idea, logging added.
From 7819293fc546de31cc5eea246242742af3be094e Mon Sep 17 00:00:00 2001
From: Pavel Vomacka <pvoma...@redhat.com>
Date: Tue, 26 Jul 2016 13:07:30 +0200
Subject: [PATCH 12/13] Coverity - accessing attribute of variable
which can
point to null

Added check whether variable is pointing to null or not.
---
install/ui/src/freeipa/widget.js | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/install/ui/src/freeipa/widget.js
b/install/ui/src/freeipa/widget.js
index
43804c5ea524ca741017d02f6e12ccf60d50b5df..1f61ce7341b1b8e13d4df5acea1f8901a63a290a
100644
--- a/install/ui/src/freeipa/widget.js
+++ b/install/ui/src/freeipa/widget.js
@@ -4938,7 +4938,7 @@ IPA.combobox_widget = function(spec) {
         var value = that.list.val();
         var option = $('option[value="'+value+'"]', that.list);
         var next = option.next();
-        if (!next.length) return;
+        if (!next || !next.length) return;
         that.select(next.val());
     };

@@ -4946,7 +4946,7 @@ IPA.combobox_widget = function(spec) {
         var value = that.list.val();
         var option = $('option[value="'+value+'"]', that.list);
         var prev = option.prev();
-        if (!prev.length) return;
+        if (!prev || !prev.length) return;
         that.select(prev.val());
     };

--
2.5.5

ACK
ACK, but IMO the situation cannot happen. .next() and .prev() should not
return null ever.

There are condition which return null in next() and prev() functions. So, it could happen.
From 3ba5110fa8b2255b83fa3e7a4135ec33b85a7fd8 Mon Sep 17 00:00:00 2001
From: Pavel Vomacka <pvoma...@redhat.com>
Date: Fri, 29 Jul 2016 10:13:21 +0200
Subject: [PATCH 13/13] Coverity - null pointer dereference

Add check which protect from calling method of null.
---
install/ui/src/freeipa/widgets/APIBrowserWidget.js | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/install/ui/src/freeipa/widgets/APIBrowserWidget.js
b/install/ui/src/freeipa/widgets/APIBrowserWidget.js
index
18773536d3587cdeb9e5fecedcc5e42c05bfe120..2164df2f5ffa00edf9ac41fd4cf6254f6d4eb9a3
100644
--- a/install/ui/src/freeipa/widgets/APIBrowserWidget.js
+++ b/install/ui/src/freeipa/widgets/APIBrowserWidget.js
@@ -264,7 +264,7 @@ widgets.APIBrowserWidget = declare([Stateful,
Evented], {
         this.list_w.select(item);

         // set item
-        widget.set('item', item);
+        if (widget) widget.set('item', item);
         this.set('current', {
             item: item,
             type: type,
--
2.5.5

ACK

Does it fix the issue? There is a line before this one which also uses
`widget`

   if (!widget.el) widget.render();

maybe we miss `return;` in:

         } else {
             IPA.notify("Invalid type", 'error');
             this.show_default();
         }





There is another patch, which fixes the line above this one (0089). Or we can add return to the and of else branch.




--
Pavel^3 Vomacka

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to