Re: [ros-dev] [ros-diffs] [ekohl] 68313: [NTOSKRNL] Add CmpDestroySecurityCache() and CmpDestroyHiveViewList() stubs and call them in CmpDestroyHive and CmUnloadKey(). CORE-6492 #resolve #comment Thank you Hermes! This is e...

[Thomas Faber]

On 2015-06-29 20:26, ekohl(a)svn.reactos.org wrote:
>   Modified: trunk/reactos/ntoskrnl/config/cmmapvw.c
>  URL:
> http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/config/cmmapvw.c?…
>  ==============================================================================
>  --- trunk/reactos/ntoskrnl/config/cmmapvw.c	[iso-8859-1] (original)
>  +++ trunk/reactos/ntoskrnl/config/cmmapvw.c	[iso-8859-1] Mon Jun 29 18:26:56 2015
>  @@ -29,3 +29,54 @@
>        Hive->PinnedViews = 0;
>        Hive->UseCount = 0;
>    }
>  +
>  +VOID
>  +NTAPI
>  +CmpDestroyHiveViewList(IN PCMHIVE Hive)
>  +{
>  +    PCM_VIEW_OF_FILE CmView;
>  +    PLIST_ENTRY EntryList;
>  +
>  +    /* Do NOT destroy the views of read-only hives */
>  +    ASSERT(Hive->Hive.ReadOnly == FALSE);
>  +
>  +    /* Free all the views inside the Pinned View List */
>  +    EntryList = RemoveHeadList(&Hive->PinViewListHead);
>  +    while (EntryList != &Hive->PinViewListHead) 
In case you haven't found it yourself yet maybe I can speed things up in
identifying the test failures here:
I made RemoveHeadList on an empty list cause a security check failure
a while back because when done unintentionally it can indicate a bug in
the code, while OTOH it's super easy to avoid.
So I'm guessing this is probably the cause, and should use a
while (!IsListEmpty()) RemoveHeadList(); or similar pattern.

If you have strong feelings against this check (which MS's headers
don't do), let me know.


>   +    {
>  +        CmView = CONTAINING_RECORD(EntryList, CM_VIEW_OF_FILE, PinViewList);
>  +
>  +        /* FIXME: Unmap the view if it is mapped */
>  +
>  +        ExFreePool(CmView);
>  +
>  +        Hive->PinnedViews--;
>  +
>  +        EntryList = RemoveHeadList(&Hive->PinViewListHead);
>  +    }
>  +
>  +    /* The Pinned View List should be empty */
>  +    ASSERT(IsListEmpty(&Hive->PinViewListHead) == TRUE);
>  +    ASSERT(Hive->PinnedViews == 0);
>  +
>  +    /* Now, free all the views inside the LRU View List */
>  +    EntryList = RemoveHeadList(&Hive->LRUViewListHead);
>  +    while (EntryList != &Hive->LRUViewListHead)
>  +    {
>  +        CmView = CONTAINING_RECORD(EntryList, CM_VIEW_OF_FILE, LRUViewList);
>  +
>  +        /* FIXME: Unmap the view if it is mapped */
>  +
>  +        ExFreePool(CmView);
>  +
>  +        Hive->MappedViews--;
>  +
>  +        EntryList = RemoveHeadList(&Hive->LRUViewListHead);
>  +    }

[Eric Kohl]

Hi Thomas,

thank you for the hint!

The "while (!IsListEmpty()) RemoveHeadList();" pattern looks nice and
seems to do the job. Will test and commit it!

Regards,
Eric

Am 29.06.2015 20:51, schrieb Thomas Faber:
>   On 2015-06-29 20:26, ekohl(a)svn.reactos.org wrote:
> >   Modified:
> > trunk/reactos/ntoskrnl/config/cmmapvw.c
> >  URL:
> >
> > http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/config/cmmapvw.c?…
> >
> >  ==============================================================================
> >
> >  --- trunk/reactos/ntoskrnl/config/cmmapvw.c    [iso-8859-1] (original)
> >  +++ trunk/reactos/ntoskrnl/config/cmmapvw.c    [iso-8859-1] Mon Jun 29
> >  18:26:56 2015
> >  @@ -29,3 +29,54 @@
> >        Hive->PinnedViews = 0;
> >        Hive->UseCount = 0;
> >    }
> >  +
> >  +VOID
> >  +NTAPI
> >  +CmpDestroyHiveViewList(IN PCMHIVE Hive)
> >  +{
> >  +    PCM_VIEW_OF_FILE CmView;
> >  +    PLIST_ENTRY EntryList;
> >  +
> >  +    /* Do NOT destroy the views of read-only hives */
> >  +    ASSERT(Hive->Hive.ReadOnly == FALSE);
> >  +
> >  +    /* Free all the views inside the Pinned View List */
> >  +    EntryList = RemoveHeadList(&Hive->PinViewListHead);
> >  +    while (EntryList != &Hive->PinViewListHead) 
>  
>  In case you haven't found it yourself yet maybe I can speed things up in
>  identifying the test failures here:
>  I made RemoveHeadList on an empty list cause a security check failure
>  a while back because when done unintentionally it can indicate a bug in
>  the code, while OTOH it's super easy to avoid.
>  So I'm guessing this is probably the cause, and should use a
>  while (!IsListEmpty()) RemoveHeadList(); or similar pattern.
>  
>  If you have strong feelings against this check (which MS's headers
>  don't do), let me know.
>  
>  
> >   +    {
> >  +        CmView = CONTAINING_RECORD(EntryList, CM_VIEW_OF_FILE,
> >  PinViewList);
> >  +
> >  +        /* FIXME: Unmap the view if it is mapped */
> >  +
> >  +        ExFreePool(CmView);
> >  +
> >  +        Hive->PinnedViews--;
> >  +
> >  +        EntryList = RemoveHeadList(&Hive->PinViewListHead);
> >  +    }
> >  +
> >  +    /* The Pinned View List should be empty */
> >  +    ASSERT(IsListEmpty(&Hive->PinViewListHead) == TRUE);
> >  +    ASSERT(Hive->PinnedViews == 0);
> >  +
> >  +    /* Now, free all the views inside the LRU View List */
> >  +    EntryList = RemoveHeadList(&Hive->LRUViewListHead);
> >  +    while (EntryList != &Hive->LRUViewListHead)
> >  +    {
> >  +        CmView = CONTAINING_RECORD(EntryList, CM_VIEW_OF_FILE,
> >  LRUViewList);
> >  +
> >  +        /* FIXME: Unmap the view if it is mapped */
> >  +
> >  +        ExFreePool(CmView);
> >  +
> >  +        Hive->MappedViews--;
> >  +
> >  +        EntryList = RemoveHeadList(&Hive->LRUViewListHead);
> >  +    } 
>  
>  
>  _______________________________________________
>  Ros-dev mailing list
>  Ros-dev(a)reactos.org
>  http://www.reactos.org/mailman/listinfo/ros-dev
>