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?re…
>  
==============================================================================
>   --- 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 <pierre(a)reactos.org
>  
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 : ros-dev-bounces(a)reactos.org [mailto:[e-mail]] De la part de Pierre
Schweitzer
Envoyé : vendredi 28 septembre 2012 09:57
À : ros-dev(a)reactos.org
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?re…
>  
==============================================================================
>   --- 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 <pierre(a)reactos.org
>  
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 : ros-dev-bounces(a)reactos.org [mailto:[e-mail]] De la part de
> Pierre Schweitzer
>  Envoyé : vendredi 28 septembre 2012 09:57
>  À : ros-dev(a)reactos.org
>  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?re…
> >   
> ==============================================================================
> >   --- 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 <pierre(a)reactos.org>
System Administrator
ReactOS Foundation