* [PATCH 1/5] Check type is not NULL before access
2010-10-26 12:59 [PATCH 0/5] Fix NULL dereference Zdenek Kabelac
@ 2010-10-26 12:59 ` Zdenek Kabelac
2010-10-26 14:02 ` Alasdair G Kergon
2010-10-26 12:59 ` [PATCH 2/5] Ensure seg is nonnull Zdenek Kabelac
` (4 subsequent siblings)
5 siblings, 1 reply; 17+ messages in thread
From: Zdenek Kabelac @ 2010-10-26 12:59 UTC (permalink / raw)
To: lvm-devel
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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 1/5] Check type is not NULL before access
2010-10-26 12:59 ` [PATCH 1/5] Check type is not NULL before access Zdenek Kabelac
@ 2010-10-26 14:02 ` Alasdair G Kergon
0 siblings, 0 replies; 17+ messages in thread
From: Alasdair G Kergon @ 2010-10-26 14:02 UTC (permalink / raw)
To: lvm-devel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 2/5] Ensure seg is nonnull
2010-10-26 12:59 [PATCH 0/5] Fix NULL dereference Zdenek Kabelac
2010-10-26 12:59 ` [PATCH 1/5] Check type is not NULL before access Zdenek Kabelac
@ 2010-10-26 12:59 ` Zdenek Kabelac
2010-10-26 13:29 ` Alasdair G Kergon
2010-10-26 12:59 ` [PATCH 3/5] Ensure first is not NULL before dereference Zdenek Kabelac
` (3 subsequent siblings)
5 siblings, 1 reply; 17+ messages in thread
From: Zdenek Kabelac @ 2010-10-26 12:59 UTC (permalink / raw)
To: lvm-devel
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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 2/5] Ensure seg is nonnull
2010-10-26 12:59 ` [PATCH 2/5] Ensure seg is nonnull Zdenek Kabelac
@ 2010-10-26 13:29 ` Alasdair G Kergon
0 siblings, 0 replies; 17+ messages in thread
From: Alasdair G Kergon @ 2010-10-26 13:29 UTC (permalink / raw)
To: lvm-devel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 3/5] Ensure first is not NULL before dereference
2010-10-26 12:59 [PATCH 0/5] Fix NULL dereference Zdenek Kabelac
2010-10-26 12:59 ` [PATCH 1/5] Check type is not NULL before access Zdenek Kabelac
2010-10-26 12:59 ` [PATCH 2/5] Ensure seg is nonnull Zdenek Kabelac
@ 2010-10-26 12:59 ` Zdenek Kabelac
2010-10-26 13:52 ` Alasdair G Kergon
2010-10-26 12:59 ` [PATCH 4/5] Fix theoretical usage of NULL pointer dereference Zdenek Kabelac
` (2 subsequent siblings)
5 siblings, 1 reply; 17+ messages in thread
From: Zdenek Kabelac @ 2010-10-26 12:59 UTC (permalink / raw)
To: lvm-devel
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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 3/5] Ensure first is not NULL before dereference
2010-10-26 12:59 ` [PATCH 3/5] Ensure first is not NULL before dereference Zdenek Kabelac
@ 2010-10-26 13:52 ` Alasdair G Kergon
0 siblings, 0 replies; 17+ messages in thread
From: Alasdair G Kergon @ 2010-10-26 13:52 UTC (permalink / raw)
To: lvm-devel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 4/5] Fix theoretical usage of NULL pointer dereference
2010-10-26 12:59 [PATCH 0/5] Fix NULL dereference Zdenek Kabelac
` (2 preceding siblings ...)
2010-10-26 12:59 ` [PATCH 3/5] Ensure first is not NULL before dereference Zdenek Kabelac
@ 2010-10-26 12:59 ` Zdenek Kabelac
2010-10-26 13:37 ` Alasdair G Kergon
2010-10-26 12:59 ` [PATCH 5/5] Check for NULL pointer Zdenek Kabelac
2010-10-26 13:57 ` [PATCH 0/5] Fix NULL dereference Alasdair G Kergon
5 siblings, 1 reply; 17+ messages in thread
From: Zdenek Kabelac @ 2010-10-26 12:59 UTC (permalink / raw)
To: lvm-devel
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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 4/5] Fix theoretical usage of NULL pointer dereference
2010-10-26 12:59 ` [PATCH 4/5] Fix theoretical usage of NULL pointer dereference Zdenek Kabelac
@ 2010-10-26 13:37 ` Alasdair G Kergon
2010-10-27 10:19 ` ejt
0 siblings, 1 reply; 17+ messages in thread
From: Alasdair G Kergon @ 2010-10-26 13:37 UTC (permalink / raw)
To: lvm-devel
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
^ permalink raw reply [flat|nested] 17+ messages in thread* [PATCH 4/5] Fix theoretical usage of NULL pointer dereference
2010-10-26 13:37 ` Alasdair G Kergon
@ 2010-10-27 10:19 ` ejt
2010-10-27 10:36 ` Zdenek Kabelac
0 siblings, 1 reply; 17+ messages in thread
From: ejt @ 2010-10-27 10:19 UTC (permalink / raw)
To: lvm-devel
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
^ permalink raw reply [flat|nested] 17+ messages in thread* [PATCH 4/5] Fix theoretical usage of NULL pointer dereference
2010-10-27 10:19 ` ejt
@ 2010-10-27 10:36 ` Zdenek Kabelac
0 siblings, 0 replies; 17+ messages in thread
From: Zdenek Kabelac @ 2010-10-27 10:36 UTC (permalink / raw)
To: lvm-devel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 5/5] Check for NULL pointer
2010-10-26 12:59 [PATCH 0/5] Fix NULL dereference Zdenek Kabelac
` (3 preceding siblings ...)
2010-10-26 12:59 ` [PATCH 4/5] Fix theoretical usage of NULL pointer dereference Zdenek Kabelac
@ 2010-10-26 12:59 ` Zdenek Kabelac
2010-10-26 13:42 ` Alasdair G Kergon
2010-10-26 13:57 ` [PATCH 0/5] Fix NULL dereference Alasdair G Kergon
5 siblings, 1 reply; 17+ messages in thread
From: Zdenek Kabelac @ 2010-10-26 12:59 UTC (permalink / raw)
To: lvm-devel
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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 5/5] Check for NULL pointer
2010-10-26 12:59 ` [PATCH 5/5] Check for NULL pointer Zdenek Kabelac
@ 2010-10-26 13:42 ` Alasdair G Kergon
0 siblings, 0 replies; 17+ messages in thread
From: Alasdair G Kergon @ 2010-10-26 13:42 UTC (permalink / raw)
To: lvm-devel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 0/5] Fix NULL dereference
2010-10-26 12:59 [PATCH 0/5] Fix NULL dereference Zdenek Kabelac
` (4 preceding siblings ...)
2010-10-26 12:59 ` [PATCH 5/5] Check for NULL pointer Zdenek Kabelac
@ 2010-10-26 13:57 ` Alasdair G Kergon
2010-10-26 14:05 ` Petr Rockai
2010-10-26 14:14 ` Zdenek Kabelac
5 siblings, 2 replies; 17+ messages in thread
From: Alasdair G Kergon @ 2010-10-26 13:57 UTC (permalink / raw)
To: lvm-devel
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
^ permalink raw reply [flat|nested] 17+ messages in thread* [PATCH 0/5] Fix NULL dereference
2010-10-26 13:57 ` [PATCH 0/5] Fix NULL dereference Alasdair G Kergon
@ 2010-10-26 14:05 ` Petr Rockai
2010-10-26 14:17 ` Zdenek Kabelac
2010-10-26 14:14 ` Zdenek Kabelac
1 sibling, 1 reply; 17+ messages in thread
From: Petr Rockai @ 2010-10-26 14:05 UTC (permalink / raw)
To: lvm-devel
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.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 0/5] Fix NULL dereference
2010-10-26 14:05 ` Petr Rockai
@ 2010-10-26 14:17 ` Zdenek Kabelac
0 siblings, 0 replies; 17+ messages in thread
From: Zdenek Kabelac @ 2010-10-26 14:17 UTC (permalink / raw)
To: lvm-devel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 0/5] Fix NULL dereference
2010-10-26 13:57 ` [PATCH 0/5] Fix NULL dereference Alasdair G Kergon
2010-10-26 14:05 ` Petr Rockai
@ 2010-10-26 14:14 ` Zdenek Kabelac
1 sibling, 0 replies; 17+ messages in thread
From: Zdenek Kabelac @ 2010-10-26 14:14 UTC (permalink / raw)
To: lvm-devel
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
^ permalink raw reply [flat|nested] 17+ messages in thread