[Gluster-devel] Help needed with Coverity - How to remove tainted_data_argument?

[Niels de Vos]

On Wed, Dec 17, 2014 at 10:24:46AM +0100, Xavier Hernandez wrote:
> On 12/17/2014 09:26 AM, Niels de Vos wrote:
> >On Wed, Dec 17, 2014 at 02:26:55AM -0500, Krishnan Parthasarathi wrote:
> >>I was looking into a Coverity issue (CID 1228603) in GlusterFS.
> >>I sent a patch[1] before I fully understood why this was an issue.
> >>After searching around in the internet for explanations, I identified that
> >>the core issue was that a character buffer, storing parts of a file (external I/O),
> >>was marked tainted. This taint spread wherever the buffer was used. This seems
> >>acceptable in the context of static analysis. How do we indicate to Coverity that
> >>the 'taint' would cause no harm as speculated?
> >>
> >>[1] - Coverity fix attempt: http://review.gluster.org/#/c/9286/
> >>[2] - CID 1228603:  Use of untrusted scalar value  (TAINTED_SCALAR):
> >>       glusterd-utils.c: 2131 in glusterd_readin_file()
> >
> >If you visit https://scan.coverity.com/projects/987 you can request an
> >account and make yourself owner of this CID (enter it in the upper right
> >corner after clicking 'view defects').
> >
> >I agree that this is safe usage. Please mark this as 'intentional'.
> 
> 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?

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?

Niels

> (GF_ASSERT() should only be used for things that should never happen under
> any circumstance, and happening means something very bad has happened). Or
> at least use structures like this:
> 
>     if (type <= x) {
>         array[x] = y;
>     } else {
>         GF_ASSERT(type <= x);
>     }
> 
> Xavi