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, [e-mail] wrote:
> Modified: trunk/reactos/ntoskrnl/config/cmmapvw.c
> URL: http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/config/cmmapvw.c?r...
> ==============================================================================
> --- 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, [e-mail] wrote:
> > Modified: trunk/reactos/ntoskrnl/config/cmmapvw.c
> > URL:
> > http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/config/cmmapvw.c?r...
> >
> > ==============================================================================
> >
> > --- 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
> [e-mail]
> http://www.reactos.org/mailman/listinfo/ros-dev