Re: [ros-dev] [ros-diffs] [tfaber] 64762: [NPFS] - Don't call RtlEqualUnicodeString (paged code) while holding a spin lock. Powered by Driver Verifier.

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

What is the *root* reason for this problem (ok I’ve seen in the comment,
“paged code” and “holding spin lock”, but it doesn’t answer my question)?
Because I’ve looked at our code of RtlEqualUnicodeString and I see that it
just calls RtlCompareUnicodeString that in turns just loops over the two
memory buffers, checks their contents (and in case we check for char case,
calls RtlUpcaseUnicodeChar). And RtlCompareMemory does that, too? What I’m
missing?

Hermès.

-------------------------------------------------------------------------
Author: tfaber
Date: Thu Oct 16 16:40:13 2014
New Revision: 64762

URL: http://svn.reactos.org/svn/reactos?rev=64762&view=rev
Log:
[NPFS]
- Don't call RtlEqualUnicodeString (paged code) while holding a spin lock.
Powered by Driver Verifier.

Modified:
    trunk/reactos/drivers/filesystems/npfs/waitsup.c

Modified: trunk/reactos/drivers/filesystems/npfs/waitsup.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/drivers/filesystems/npfs/wa
itsup.c?rev=64762&r1=64761&r2=64762&view=diff
============================================================================
==
--- trunk/reactos/drivers/filesystems/npfs/waitsup.c	[iso-8859-1]
(original)
+++ trunk/reactos/drivers/filesystems/npfs/waitsup.c	[iso-8859-1] Thu Oct
16 16:40:13 2014
@@ -97,6 +97,22 @@
 {
     InitializeListHead(&WaitQueue->WaitList);
     KeInitializeSpinLock(&WaitQueue->WaitLock);
+}
+
+static
+BOOLEAN
+NpEqualUnicodeString(IN PCUNICODE_STRING String1,
+                     IN PCUNICODE_STRING String2)
+{
+    SIZE_T EqualLength;
+
+    if (String1->Length != String2->Length)
+        return FALSE;
+
+    EqualLength = RtlCompareMemory(String1->Buffer,
+                                   String2->Buffer,
+                                   String1->Length);
+    return EqualLength == String1->Length;
 }

NTSTATUS
@@ -156,7 +172,8 @@
             PipeName.MaximumLength = PipeName.Length;
         }

-        if (RtlEqualUnicodeString(&WaitName, &PipeName, FALSE))
+        /* Can't use RtlEqualUnicodeString with a spinlock held */
+        if (NpEqualUnicodeString(&WaitName, &PipeName))
         {
             /* Found a matching wait. Cancel it */
             RemoveEntryList(&WaitIrp->Tail.Overlay.ListEntry);

[Thomas Faber]

RtlEqualUnicodeString is in a pagable code section in ntoskrnl,
RtlCompareMemory is not. Holding a spinlock means we're at
DISPATCH_LEVEL and can't access pagable code/data.

On 2014-10-16 19:32, Hermès BÉLUSCA - MAÏTO wrote:
> What is the *root* reason for this problem (ok I’ve seen in the comment,
> “paged code” and “holding spin lock”, but it doesn’t answer my question)?
> Because I’ve looked at our code of RtlEqualUnicodeString and I see that it
> just calls RtlCompareUnicodeString that in turns just loops over the two
> memory buffers, checks their contents (and in case we check for char case,
> calls RtlUpcaseUnicodeChar). And RtlCompareMemory does that, too? What I’m
> missing?
>
> Hermès.
>
> Author: tfaber
> Date: Thu Oct 16 16:40:13 2014
> New Revision: 64762
>
> URL: http://svn.reactos.org/svn/reactos?rev=64762&view=rev
> Log:
> [NPFS]
> Don't call RtlEqualUnicodeString (paged code) while holding a spin lock.
>
> Powered by Driver Verifier.
>
> Modified:
>     trunk/reactos/drivers/filesystems/npfs/waitsup.c
>
> Modified: trunk/reactos/drivers/filesystems/npfs/waitsup.c
> URL:
> http://svn.reactos.org/svn/reactos/trunk/reactos/drivers/filesystems/npfs/wa
> itsup.c?rev=64762&r1=64761&r2=64762&view=diff
> ============================================================================
> ==
> --- trunk/reactos/drivers/filesystems/npfs/waitsup.c	[iso-8859-1]
> (original)
> +++ trunk/reactos/drivers/filesystems/npfs/waitsup.c	[iso-8859-1] Thu Oct
> 16 16:40:13 2014
> @@ -97,6 +97,22 @@
>  {
>      InitializeListHead(&WaitQueue->WaitList);
>      KeInitializeSpinLock(&WaitQueue->WaitLock);
> +}
>
> +static
> +BOOLEAN
> +NpEqualUnicodeString(IN PCUNICODE_STRING String1,
>                 IN PCUNICODE_STRING String2)
>
> +{
> SIZE_T EqualLength;
> if (String1->Length != String2->Length)
>    return FALSE;
> EqualLength = RtlCompareMemory(String1->Buffer,
>                               String2->Buffer,
>                               String1->Length);
> return EqualLength == String1->Length;
>
> }
>
> NTSTATUS
> @@ -156,7 +172,8 @@
>              PipeName.MaximumLength = PipeName.Length;
>          }
>    if (RtlEqualUnicodeString(&WaitName, &PipeName, FALSE))
>    /* Can't use RtlEqualUnicodeString with a spinlock held */
>    if (NpEqualUnicodeString(&WaitName, &PipeName))
>    {
>        /* Found a matching wait. Cancel it */
>        RemoveEntryList(&WaitIrp->Tail.Overlay.ListEntry);