[PATCH 0/5] Fix NULL dereference

[PATCH 0/5] Fix NULL dereference

[Zdenek Kabelac]

Updated patchset for NULL pointer dereferences issues reported by clang.

Unlike the first version - this time less aggresive solution is used.
INTERNAL_ERRORs are reported in these moments (if they would ever happen),
and the execution path aborts when such conditions are met.
Previous version was rather ignoring these paths and could lead to 
unwanted execution of other code parts.


Zdenek Kabelac (5):
  Check type is not NULL before access
  Ensure seg is nonnull
  Ensure  first is not NULL before dereference
  Fix theoretical usage of NULL pointer dereference
  Check for NULL pointer

 lib/activate/dev_manager.c |    5 +++++
 lib/format1/format1.c      |    5 +++++
 libdm/libdm-report.c       |    6 ++++++
 libdm/regex/ttree.c        |    6 ++++++
 tools/reporter.c           |    4 ++++
 5 files changed, 26 insertions(+), 0 deletions(-)

-- 
1.7.3.2

[PATCH 1/5] Check type is not NULL before access

[Zdenek Kabelac]

clang Logic error Dereference of null pointer

Check type before dereference and report internal error in case
it's undefined and return from the function.

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 libdm/libdm-report.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/libdm/libdm-report.c b/libdm/libdm-report.c
index 7631e21..db24fed 100644
--- a/libdm/libdm-report.c
+++ b/libdm/libdm-report.c
@@ -273,6 +273,12 @@ static void _display_fields(struct dm_report *rh)
 			log_warn("%*.*s", (int) strlen(desc) + 7,
 				 (int) strlen(desc) + 7,
 				 "-------------------------------------------------------------------------------");
+
+			if (!type) {
+				log_error(INTERNAL_ERROR "Type is not defined.");
+				return;
+			}
+
 			log_warn("  %sall%-*s - %s", type->prefix,
 				 (int) (id_len - 3 - strlen(type->prefix)), "",
 				 "All fields in this section.");
-- 
1.7.3.2

[PATCH 2/5] Ensure seg is nonnull

[Zdenek Kabelac]

clang Logic error	Dereference of null pointer
Make sure we do not try to use NULL seg pointer.

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 lib/activate/dev_manager.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/lib/activate/dev_manager.c b/lib/activate/dev_manager.c
index cbb5378..cc67a60 100644
--- a/lib/activate/dev_manager.c
+++ b/lib/activate/dev_manager.c
@@ -626,6 +626,11 @@ int dev_manager_transient(struct dev_manager *dm, struct logical_volume *lv)
 		if (!type || !params)
 			continue;
 
+		if (!seg) {
+			log_error(INTERNAL_ERROR "Segment is not selected.");
+			goto out;
+		}
+
 		if (seg->segtype->ops->check_transient_status &&
 		    !seg->segtype->ops->check_transient_status(seg, params))
 			goto_out;
-- 
1.7.3.2

[PATCH 3/5] Ensure first is not NULL before dereference

[Zdenek Kabelac]

clang Logic error Dereference of null pointer

For empty lists make sure the NULL first pointer is not dereferenced
and report error.

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 lib/format1/format1.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/lib/format1/format1.c b/lib/format1/format1.c
index 0dcec05..c5685c9 100644
--- a/lib/format1/format1.c
+++ b/lib/format1/format1.c
@@ -102,6 +102,11 @@ static int _check_vgs(struct dm_list *pvs, struct volume_group *vg)
 		pv_count++;
 	}
 
+	if (!first) {
+		log_error(INTERNAL_ERROR "Unexpected empty PV list.");
+		return 0;
+	}
+
 	/* On entry to fn, list known to be non-empty */
 	if (pv_count != first->vgd.pv_cur) {
 		log_error("%d PV(s) found for VG %s: expected %d",
-- 
1.7.3.2

[PATCH 4/5] Fix theoretical usage of NULL pointer dereference

[Zdenek Kabelac]

Make sure we have *c defined.
clang seems to be happier with this check.

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 libdm/regex/ttree.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/libdm/regex/ttree.c b/libdm/regex/ttree.c
index ec97c98..a384c59 100644
--- a/libdm/regex/ttree.c
+++ b/libdm/regex/ttree.c
@@ -97,6 +97,12 @@ int ttree_insert(struct ttree *tt, unsigned int *key, void *data)
 			}
 		}
 	}
+
+	if (!*c) {
+		log_error(INTERNAL_ERROR "Insert failed.");
+		return 0;
+	}
+
 	(*c)->data = data;
 
 	return 1;
-- 
1.7.3.2

[PATCH 5/5] Check for NULL pointer

[Zdenek Kabelac]

clangs is happier and check for non NULL options value.
Report internal error in this case.

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 tools/reporter.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/tools/reporter.c b/tools/reporter.c
index f39d23f..24a27ba 100644
--- a/tools/reporter.c
+++ b/tools/reporter.c
@@ -324,6 +324,10 @@ static int _report(struct cmd_context *cmd, int argc, char **argv,
 			return EINVALID_CMD_LINE;
 		}
 		if (*opts == '+') {
+			if (!options) {
+				log_error(INTERNAL_ERROR "Missing options.");
+				return EINVALID_CMD_LINE;
+			}
 			if (!(str = dm_pool_alloc(cmd->mem,
 					 strlen(options) + strlen(opts) + 1))) {
 				log_error("options string allocation failed");
-- 
1.7.3.2

[PATCH 2/5] Ensure seg is nonnull

[Alasdair G Kergon]

On Tue, Oct 26, 2010 at 02:59:23PM +0200, Zdenek Kabelac wrote:
> +		if (!seg) {
> +			log_error(INTERNAL_ERROR "Segment is not selected.");

I don't mind assertions like this, but please describe the problem more
accurately in the message.  If this triggers, doesn't it mean we
have a corrupt segment list?  So say that.
"LV %s segment list corrupted: NULL struct lv_segment detected."

But isn't there a missing 'if (lv)' ?

Alasdair

[PATCH 4/5] Fix theoretical usage of NULL pointer dereference

[Alasdair G Kergon]

On Tue, Oct 26, 2010 at 02:59:25PM +0200, Zdenek Kabelac wrote:
> @@ -97,6 +97,12 @@ int ttree_insert(struct ttree *tt, unsigned int *key, void *data)

> +	if (!*c) {
> +		log_error(INTERNAL_ERROR "Insert failed.");

What am I missing here?
Isn't that condition you are proposing to add logically impossible to
trigger?!

It's not our job to add code just because a particular static analyser
isn't clever enough to understand the logic.

Nack.

Alasdair

[PATCH 5/5] Check for NULL pointer

[Alasdair G Kergon]

On Tue, Oct 26, 2010 at 02:59:26PM +0200, Zdenek Kabelac wrote:
> +			if (!options) {
> +				log_error(INTERNAL_ERROR "Missing options.");
> +				return EINVALID_CMD_LINE;
> +			}

Again - what is the bug here?

options is always non-NULL there surely?

What code path causes that new check to fire?

Nack without a decent explanation in the error message of the *cause* not
the symptom.

Alasdair

[PATCH 3/5] Ensure first is not NULL before dereference

[Alasdair G Kergon]

On Tue, Oct 26, 2010 at 02:59:24PM +0200, Zdenek Kabelac wrote:
> +	if (!first) {
> +		log_error(INTERNAL_ERROR "Unexpected empty PV list.");
> +		return 0;

We already have a 'list empty' check prior to calling this function.

I think it's more readable with the check in the caller and the
comment inside the function, instead of moving the check inside
the function.

No need to change this that I can see.

What is the code path that could trigger this condition?
(It's a static function only called from one place.)

What am I missing?

Alasdair

[PATCH 0/5] Fix NULL dereference

[Alasdair G Kergon]

On Tue, Oct 26, 2010 at 02:59:21PM +0200, Zdenek Kabelac wrote:
> Updated patchset for NULL pointer dereferences issues reported by clang.
> 
> Unlike the first version - this time less aggresive solution is used.
> INTERNAL_ERRORs are reported in these moments (if they would ever happen),
> and the execution path aborts when such conditions are met.
> Previous version was rather ignoring these paths and could lead to 
> unwanted execution of other code parts.
 
Well the ones I've looked at here seem to be more about dealing with
shortcomings in the static analysis code rather than fixing real bugs.

Alasdair

[PATCH 1/5] Check type is not NULL before access

[Alasdair G Kergon]

On Tue, Oct 26, 2010 at 02:59:22PM +0200, Zdenek Kabelac wrote:
> Check type before dereference and report internal error in case
> it's undefined and return from the function.
 
> +			if (!type) {
> +				log_error(INTERNAL_ERROR "Type is not defined.");
> +				return;
> +			}

Is this one really an internal library error?

Can it be triggered if someone usees the API incorrectly?

If so, it should be treated like validation.

Alasdair

[PATCH 0/5] Fix NULL dereference

[Petr Rockai]

Hi,

Alasdair G Kergon <agk@redhat.com> writes:
> Well the ones I've looked at here seem to be more about dealing with
> shortcomings in the static analysis code rather than fixing real bugs.
talking about static analysis. We used to have access to Coverity
Prevent reports about LVM, right? If I try to go to scan.coverity.com
and click "sign in" near the LVM2 project, I get a connection
refused... the link is sending me to

http://scan2.coverity.com:7494/

Alasdair, I suppose you are the Coverity's "contact" -- could you maybe
ask what's up with that, and maybe get me an account so I can see the
report as well? I reckon it could be useful...

Yours,
   Petr.

[PATCH 0/5] Fix NULL dereference

[Zdenek Kabelac]

Dne 26.10.2010 15:57, Alasdair G Kergon napsal(a):
> On Tue, Oct 26, 2010 at 02:59:21PM +0200, Zdenek Kabelac wrote:
>> Updated patchset for NULL pointer dereferences issues reported by clang.
>>
>> Unlike the first version - this time less aggresive solution is used.
>> INTERNAL_ERRORs are reported in these moments (if they would ever happen),
>> and the execution path aborts when such conditions are met.
>> Previous version was rather ignoring these paths and could lead to 
>> unwanted execution of other code parts.
>  
> Well the ones I've looked at here seem to be more about dealing with
> shortcomings in the static analysis code rather than fixing real bugs.
> 


Some of them can never be triggered within current LVM code.
Static analyzer is currently incapable to model data structure behavior
to understand, that some settings can never happen and sometimes it creates
very complex code path to model NULL pointer at the end.
(Also instrumentation  nonnull would be handy here - but it's long term goal)

However my small patches here really just try to clean warning - the price for
checks seems to be quite low  and we do not need to look into analyzer output
again and again.

We may also put them into

#ifdef __clang__
#endif

section to avoid any runtime overheads - but I don't like spreading such
ifdefs everywhere.

I can also keep these patches in my private branch - to not be always bothered
with same error.

For now I did not want to spend too much time on this so I've rather fixed
easily and quickly what I've considered to be even worth to look at.

Of course deeper analysis here will require some time - so - placing them to
my low-prio background queue....

Zdenek

[PATCH 0/5] Fix NULL dereference

[Zdenek Kabelac]

Dne 26.10.2010 16:05, Petr Rockai napsal(a):
> Hi,
> 
> Alasdair G Kergon <agk@redhat.com> writes:
>> Well the ones I've looked at here seem to be more about dealing with
>> shortcomings in the static analysis code rather than fixing real bugs.
> talking about static analysis. We used to have access to Coverity
> Prevent reports about LVM, right? If I try to go to scan.coverity.com
> and click "sign in" near the LVM2 project, I get a connection
> refused... the link is sending me to
> 
> http://scan2.coverity.com:7494/
> 
> Alasdair, I suppose you are the Coverity's "contact" -- could you maybe
> ask what's up with that, and maybe get me an account so I can see the
> report as well? I reckon it could be useful...
> 


I've tried  4 times to get any answer from then - never got any responce.
So I've considered they probably stopped to give access to their results.

Zdenek

[PATCH 4/5] Fix theoretical usage of NULL pointer dereference

[ejt]

At Tue, 26 Oct 2010 14:37:43 +0100,
Alasdair G Kergon wrote:
> 
> On Tue, Oct 26, 2010 at 02:59:25PM +0200, Zdenek Kabelac wrote:
> > @@ -97,6 +97,12 @@ int ttree_insert(struct ttree *tt, unsigned int *key, void *data)
> 
> > +	if (!*c) {
> > +		log_error(INTERNAL_ERROR "Insert failed.");
> 
> What am I missing here?
> Isn't that condition you are proposing to add logically impossible to
> trigger?!

Yes, it can't happen, as I said the first time this patch went round.

One thing I'm not clear on is how much benefit we're seeing from the
CLang build?  Kabi, how many genuine bugs did you find when you went
through this process?  If the benefits are real, then we can live with
check like these.  It would be nice if it was clearer that they are
only there to pacify clang, maybe put a conditional compile in so
they're only included with the clang build?

- Joe

[PATCH 4/5] Fix theoretical usage of NULL pointer dereference

[Zdenek Kabelac]

Dne 27.10.2010 12:19, ejt at redhat.com napsal(a):
> At Tue, 26 Oct 2010 14:37:43 +0100,
> Alasdair G Kergon wrote:
>>
>> On Tue, Oct 26, 2010 at 02:59:25PM +0200, Zdenek Kabelac wrote:
>>> @@ -97,6 +97,12 @@ int ttree_insert(struct ttree *tt, unsigned int *key, void *data)
>>
>>> +	if (!*c) {
>>> +		log_error(INTERNAL_ERROR "Insert failed.");
>>
>> What am I missing here?
>> Isn't that condition you are proposing to add logically impossible to
>> trigger?!
> 
> Yes, it can't happen, as I said the first time this patch went round.
> 
> One thing I'm not clear on is how much benefit we're seeing from the
> CLang build?  Kabi, how many genuine bugs did you find when you went
> through this process?  If the benefits are real, then we can live with
> check like these.  It would be nice if it was clearer that they are
> only there to pacify clang, maybe put a conditional compile in so
> they're only included with the clang build?

>From the first original patchset real bugs are already commited.
I think patches 4, 5, 10, 14, 15.  (Some of them are clearly problems of
missing deep unit tests probably - but running scan-build - or spending
months writing tests for every single error path in the code - I think we do
not have manpower for this at this moment...)

The problem here is not what would happen if everything goes 'right', but what
could happen if something goes 'wrong' - i.e. we may overwrite
some bytes in memory by some other errors, we may leave some structure in
wrong state, because of some unchecked error path - we could misuse or wrongly
reuse something- obviously we will need to find the real cause of such memory
overwrite - but the question is - is it the best thing to generate a coredump
- or should we nicely bailout from such case  and give user some  error report?

I didn't want to spend too much time with this thing in first place - so I've
chosen easiest path here - and as mention in the patchset header - under
normal circumstances lots of those (sometimes really crazy code paths) are not
reachable - but if these checks are so cheap - why not add them -  we do not
mask the bug - we just avoid coredump for this case.

Zdenek