[Gluster-devel] Move to a more strict GF_ASSERT() checking? (WAS: Help needed with Coverity)

[Niels de Vos]

On Wed, Dec 17, 2014 at 11:39:59AM +0100, Xavier Hernandez wrote:
> On 12/17/2014 11:29 AM, Niels de Vos wrote:
> >On Wed, Dec 17, 2014 at 10:24:46AM +0100, Xavier Hernandez wrote:
> >>I think the root cause of this particular problem is a pattern like this:
> >>
> >>     GF_ASSERT(type <= x);
> >>
> >>     array[type] = y
> >>
> >>I think there are several places where this pattern is used. GF_ASSERT() is
> >>there to detect bugs, however it does not stop the execution, it simply logs
> >>the problem. So if there's really a bug and 'type' > x, we will do an
> >>incorrect access into the array. Additionally, if 'type' is obtained from a
> >>tainted pointer, coverity considers this a potential security issue.
> >>
> >>This is also applicable to NULL pointer checks and some others.
> >>
> >>Wouldn't it be better to make GF_ASSERT() to really stop the program ?
> >
> >Or, maybe we should fix these kind of GF_ASSERT() usages and call
> >GF_ASSERT_AND_GOTO_WITH_ERROR() instead?
> 
> That would be a good option also.

Okay, maybe we should look into doing it this way, and mark direct usage
of GF_ASSERT() obsolete?

> 
> >
> >I do not know how Coverity runs checks, but when DEBUG is defined,
> >GF_ASSERT() will call assert() and execution should be aborted. Maybe
> >Coverity can be configured to hit assert() instead of ignoring it?
> 
> If this can be done, coverity won't report these issues, however the
> underlying problem will be there in release compilations.

Oh, I do not know how Coverity would (if?) report reaching an assert().
I would expect Coverity complain very loud about it, but maybe it
doesnt.

> If a bug really exists that makes the assert to fail, and the argument that
> doesn't satisfy the condition is used anyway, there could be a possibility
> in some places that this leads to data corruption, which would be very bad.

Yes, and depending on how Coverity would report assert(), we can, or can
not enabled DEBUG for Coverity analysis.

Niels
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://supercolony.gluster.org/pipermail/gluster-devel/attachments/20141217/bb013c40/attachment-0001.sig>