Re: [ros-dev] [ros-diffs] [hbelusca] 57400: [NTOSKRNL] Coverity code defects fixes : - Cache: CID 701441 - Config: CIDs 716570, 716669, 716760 - Dbgk: Kdbg: CIDs 716571, 515128/9, 500432 - Ex: CIDs 500156/7, 515122, 716200/...

[Pierre Schweitzer]

What's the purpose of such ASSERT?
> Modified: trunk/reactos/ntoskrnl/fsrtl/notify.c
> URL:

http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/fsrtl/notify.c?rev...
>

==============================================================================
> --- trunk/reactos/ntoskrnl/fsrtl/notify.c [iso-8859-1] (original)
> +++ trunk/reactos/ntoskrnl/fsrtl/notify.c [iso-8859-1] Thu Sep 27

17:16:31 2012
> @@ -586,6 +586,13 @@
> /* Allocate new notification */
> NotifyChange = ExAllocatePoolWithTag(PagedPool |

POOL_RAISE_IF_ALLOCATION_FAILURE,
> sizeof(NOTIFY_CHANGE), 'FSrN');
> /*
> If NotifyChange == NULL then an
> exception was already raised.
> */
> ASSERT(NotifyChange != NULL);
>
> RtlZeroMemory(NotifyChange, sizeof(NOTIFY_CHANGE));
>
> /* Set basic information */
-- 
Pierre Schweitzer [e-mail]
System Administrator
ReactOS Foundation

[Hermès BÉLUSCA - MAÏTO]

Hi !

It's an attempt to say to Coverity that the NotifyChange variable will NEVER be NULL after the allocation (as explained in the comment), because of the fact that the code uses POOL_RAISE_IF_ALLOCATION_FAILURE. Indeed, Coverity expected a "check against NULL and fail if it is" before the usage of the NotifyChange variable and therefore warned about its usage without checking against NULL, because it's dereferenced just after the allocation call. Coverity couldn't know what the usage of the POOL_RAISE_IF_ALLOCATION_FAILURE is for. In conclusion, the aim of the assert is to remove the Coverity warning without adding code-behaviour modification.

Hermès.

-----Message d'origine-----
De : [e-mail] [mailto:[e-mail]] De la part de Pierre Schweitzer
Envoyé : vendredi 28 septembre 2012 09:57
À : [e-mail]
Objet : Re: [ros-dev] [ros-diffs] [hbelusca] 57400: [NTOSKRNL] Coverity code defects fixes : - Cache: CID 701441 - Config: CIDs 716570, 716669, 716760 - Dbgk: Kdbg: CIDs 716571, 515128/9, 500432 - Ex: CIDs 500156/7, 515122, 716200/...

What's the purpose of such ASSERT?
> Modified: trunk/reactos/ntoskrnl/fsrtl/notify.c
> URL:

http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/fsrtl/notify.c?rev...
>

==============================================================================
> --- trunk/reactos/ntoskrnl/fsrtl/notify.c [iso-8859-1] (original)
> +++ trunk/reactos/ntoskrnl/fsrtl/notify.c [iso-8859-1] Thu Sep 27

17:16:31 2012
> @@ -586,6 +586,13 @@
> /* Allocate new notification */
> NotifyChange = ExAllocatePoolWithTag(PagedPool |

POOL_RAISE_IF_ALLOCATION_FAILURE,
> sizeof(NOTIFY_CHANGE), 'FSrN');
> /*
> If NotifyChange == NULL then an
> exception was already raised.
> */
> ASSERT(NotifyChange != NULL);
>
> RtlZeroMemory(NotifyChange, sizeof(NOTIFY_CHANGE));
>
> /* Set basic information */
-- 
Pierre Schweitzer [e-mail]
System Administrator
ReactOS Foundation

[Pierre Schweitzer]

Then, you'd rather mark it as false-positive in Coverity interface
rather than adding redundant and useless check.

Or, if Coverity supports it, annotate code.

The issue here is Coverity, not the code.

On Fri, 2012-09-28 at 11:34 +0200, Hermès BÉLUSCA - MAÏTO wrote:
> Hi !
>
> It's an attempt to say to Coverity that the NotifyChange variable will NEVER be NULL after the allocation (as explained in the comment), because of the fact that the code uses POOL_RAISE_IF_ALLOCATION_FAILURE. Indeed, Coverity expected a "check against NULL and fail if it is" before the usage of the NotifyChange variable and therefore warned about its usage without checking against NULL, because it's dereferenced just after the allocation call. Coverity couldn't know what the usage of the POOL_RAISE_IF_ALLOCATION_FAILURE is for. In conclusion, the aim of the assert is to remove the Coverity warning without adding code-behaviour modification.
>
> Hermès.
>
> -----Message d'origine-----
> De : [e-mail] [mailto:[e-mail]] De la part de Pierre Schweitzer
> Envoyé : vendredi 28 septembre 2012 09:57
> À : [e-mail]
> Objet : Re: [ros-dev] [ros-diffs] [hbelusca] 57400: [NTOSKRNL] Coverity code defects fixes : - Cache: CID 701441 - Config: CIDs 716570, 716669, 716760 - Dbgk: Kdbg: CIDs 716571, 515128/9, 500432 - Ex: CIDs 500156/7, 515122, 716200/...
>
> What's the purpose of such ASSERT?
> > Modified: trunk/reactos/ntoskrnl/fsrtl/notify.c
> > URL:
>
> http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/fsrtl/notify.c?rev...
> >
>
> ==============================================================================
> > --- trunk/reactos/ntoskrnl/fsrtl/notify.c [iso-8859-1] (original)
> > +++ trunk/reactos/ntoskrnl/fsrtl/notify.c [iso-8859-1] Thu Sep 27
>
> 17:16:31 2012
> > @@ -586,6 +586,13 @@
> > /* Allocate new notification */
> > NotifyChange = ExAllocatePoolWithTag(PagedPool |
>
> POOL_RAISE_IF_ALLOCATION_FAILURE,
> > sizeof(NOTIFY_CHANGE), 'FSrN');
> > /*
> > If NotifyChange == NULL then an
> > exception was already raised.
> > */
> > ASSERT(NotifyChange != NULL);
> >
> > RtlZeroMemory(NotifyChange, sizeof(NOTIFY_CHANGE));
> >
> > /* Set basic information */
-- 
Pierre Schweitzer [e-mail]
System Administrator
ReactOS Foundation