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

[Xavier Hernandez]

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 ? 
(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