Commenting only on top, it's too long. ACK for everything. I've rebased patch 90.
pushed to master master: * a2525ff64518038eaa64b0d855154a984030f7f3 Coverity - null pointer exception * d4ad0ca04c0ae445c784787a675ac84d2cbfd766 Coverity - null pointer exception * fa3982c7c82add3d201aec860cb981a595f10be9 Coverity - not initialized variable * de8cb7585b652fd1a61e3020e37192cb1db74f46 Coverity - identical code for different branches * 4b63ce26ebbef8ef1538aecb3cff8032df3357a7 Coverity - Accesing attribute of null * ed74e14ab4a17c83cf6782e4b6fd41a2ce79594d Coverity - removed dead code * 7be585dbb206ed12b25d09bfb2f5452ee9c125ae Coverity - true branch can't be executed * d94a2aa185defba38f2bbe2c5ee28f9b9defc0f2 Coverity - true branch can't be executed * cad9f9b682d9bcc33fdfb1112e4cfb1a2c66a498 Coverity - null pointer dereference * 4af31c70c57fc223920b71fedfb40d1de27622b2 Coverity - iterating over variable which could be null * cd74f78ed74f8898c492024d0901cef9778df067 Coverity - opens dialog which might not be created * aa8a904c4a3953e799278de192d1613d21cde42a Coverity - accessing attribute of variable which can point to null * 2644c955489ee5b22ecc0227c5cd8ed1e90ee648 Coverity - null pointer dereference 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 <[email protected]> >>>> 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 <[email protected]> >>>> 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 <[email protected]> >>>> 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 <[email protected]> >>>> 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 <[email protected]> >>>> 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 <[email protected]> >>>> 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 <[email protected]> >>>> 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 <[email protected]> >>>> 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 <[email protected]> >>>> 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 <[email protected]> >>>> 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 <[email protected]> >>>> 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 <[email protected]> >>>> 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 <[email protected]> >>>> 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. > -- Petr Vobornik -- 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
