From: Paul Durrant <pdurrant@amazon.com>
Date: Wed, 17 Feb 2021 18:25:39 +0000 (+0000)
Subject: Make sure StoreSubmitRequest() cannot fail...
X-Git-Url: http://xenbits.xensource.com/gitweb?a=commitdiff_plain;h=4d3c233a51aef91d464db4d295a1d76ef774a27d;p=pvdrivers%2Fwin%2Fxenbus.git

Make sure StoreSubmitRequest() cannot fail...

... after a request completes successfully in xenstored.

Currently a failure is possible if a request completes successfully but
StoreCopyResponse() fails to allocate memory. This has a particularly nasty
side effect in StoreTransactionStart() where is can return a failure status
to its caller but a new transaction was, in fact, initialized in xenstored.
This then leads to a transaction 'leak'.

This patch makes sure that memory is allocated up-front in
StoreSubmitRequest() so it cannot fail after communicating with xenstored.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---

diff --git a/src/xenbus/store.c b/src/xenbus/store.c
index 2f9f365..ce4c755 100644
--- a/src/xenbus/store.c
+++ b/src/xenbus/store.c
@@ -739,21 +739,15 @@ StoreResetResponse(
     Segment->Length = sizeof (struct xsd_sockmsg);
 }
 
-static PXENBUS_STORE_RESPONSE
+static VOID
 StoreCopyResponse(
-    IN  PXENBUS_STORE_CONTEXT   Context
+    IN  PXENBUS_STORE_CONTEXT   Context,
+    OUT PXENBUS_STORE_RESPONSE  Response
     )
 {
-    PXENBUS_STORE_RESPONSE      Response;
     PXENBUS_STORE_SEGMENT       Segment;
-    NTSTATUS                    status;
-
-    Response = __StoreAllocate(sizeof (XENBUS_STORE_RESPONSE));
-
-    status = STATUS_NO_MEMORY;
-    if (Response == NULL)
-        goto fail1;
 
+    ASSERT(Response != NULL);
     *Response = Context->Response;
 
     Segment = &Response->Segment[XENBUS_STORE_RESPONSE_HEADER_SEGMENT];
@@ -767,13 +761,6 @@ StoreCopyResponse(
     } else {
         ASSERT3P(Segment->Data, ==, NULL);
     }
-
-    return Response;
-
-fail1:
-    Error("fail1 (%08x)\n", status);
-
-    return NULL;
 }
 
 static VOID
@@ -817,7 +804,7 @@ StoreProcessResponse(
 
     RemoveEntryList(&Request->ListEntry);
 
-    Request->Response = StoreCopyResponse(Context);
+    StoreCopyResponse(Context, Request->Response);
     StoreResetResponse(Context);
 
     Request->State = XENBUS_STORE_REQUEST_COMPLETED;
@@ -921,9 +908,16 @@ StoreSubmitRequest(
     ULONG                       Count;
     XENBUS_STORE_REQUEST_STATE  State;
     LARGE_INTEGER               Timeout;
+    NTSTATUS                    status;
 
     ASSERT3U(Request->State, ==, XENBUS_STORE_REQUEST_PREPARED);
 
+    Request->Response = __StoreAllocate(sizeof (XENBUS_STORE_RESPONSE));
+
+    status = STATUS_NO_MEMORY;
+    if (Request->Response == NULL)
+        goto fail1;
+
     // Make sure we don't suspend
     ASSERT3U(KeGetCurrentIrql(), <=, DISPATCH_LEVEL);
     KeRaiseIrql(DISPATCH_LEVEL, &Irql);
@@ -942,8 +936,6 @@ StoreSubmitRequest(
     Timeout.QuadPart = TIME_RELATIVE(TIME_S(XENBUS_STORE_POLL_PERIOD));
 
     while (State != XENBUS_STORE_REQUEST_COMPLETED) {
-        NTSTATUS    status;
-
         status = XENBUS_EVTCHN(Wait,
                                &Context->EvtchnInterface,
                                Context->Channel,
@@ -970,6 +962,11 @@ StoreSubmitRequest(
     KeLowerIrql(Irql);
 
     return Response;
+
+fail1:
+    Error("fail1 (%08x)\n", status);
+
+    return NULL;
 }
 
 static NTSTATUS