Re: [PATCH 3/3] Fix issues raised by CodeQL

[Paul Durrant <xadimgnik@xxxxxxxxx>]

On 31/08/2021 08:23, Owen Smith wrote:
> -----Original Message-----
> From: win-pv-devel <win-pv-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf Of 
> Paul Durrant
> Sent: Friday, August 20, 2021 9:15 AM
> To: win-pv-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH 3/3] Fix issues raised by CodeQL
>
> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments 
> unless you have verified the sender and know the content is safe.
>
> On 10/08/2021 16:40, Owen Smith wrote:
> > - ExAllocatePoolWithTag is deprecated in Windows 10 2004 and replaced with
> >       ExAllocatePool2. Use ExAllocatePoolUninitialized to maintain support for
> >       earlier versions of Windows.
> > - strtol can fail, check for LONG_MIN or LONG_MAX as indicators of an
> > error
> I think these probably ought to be separate patches. As for the strtol() 
> change... There also uses of strtoul() (and _strtoui64()) elsewhere. If we're 
> going to catch overflow here then ought we not have similar checking there?
>
>     Paul
>
> Only strtol() was flagged as a warning by the newer compiler, not strtoul() or 
> _strtoui64(). I only fixed the minimum to get the compile to stop raising 
> warnings
Ok, I see why now. I've re-worked that part of the patch as follows:

-----
diff --git a/src/xenfilt/emulated.c b/src/xenfilt/emulated.c
index 0c2c08761875..83b20704a642 100644
--- a/src/xenfilt/emulated.c
+++ b/src/xenfilt/emulated.c
@@ -171,30 +171,30 @@ EmulatedSetObjectDiskData(
     if (Type != XENFILT_EMULATED_OBJECT_TYPE_IDE)
         goto fail1;

-    Controller = strtol(InstanceID, &End, 10);
+    Controller = strtoul(InstanceID, &End, 10);

     status = STATUS_INVALID_PARAMETER;
-    if (*End != '.' || Controller > 1)
+    if (Controller > 1 || *End != '.')
         goto fail2;

     End++;

-    Target = strtol(End, &End, 10);
+    Target = strtoul(End, &End, 10);

     status = STATUS_INVALID_PARAMETER;
-    if (*End != '.' || Target > 1)
+    if (Target > 1 || *End != '.')
         goto fail3;

     End++;

-    Lun = strtol(End, &End, 10);
-
-    status = STATUS_INVALID_PARAMETER;
-    if (*End != '\0')
-        goto fail4;
+    Lun = strtoul(End, &End, 10);

     status = STATUS_NOT_SUPPORTED;
     if (Lun != 0)
+        goto fail4;
+
+    status = STATUS_INVALID_PARAMETER;
+    if (*End != '\0')
         goto fail5;

     EmulatedObject->Data.Disk.Index = Controller << 1 | Target;
-----

This is somewhat less invasive.

  Paul