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 _______________________________________________ Gluster-devel mailing list Gluster-devel@xxxxxxxxxxx http://supercolony.gluster.org/mailman/listinfo/gluster-devel
