Re: [PATCH] Fix ASSERT failures in error paths

[Paul Durrant <xadimgnik@xxxxxxxxx>]

On 30/01/2023 14:42, Owen Smith wrote:
> * StorePrepareRequest can fail if a non-NULL transaction is passed.
>    Check the return value where a transaction is passed to StorePrepareRequest
> * StoreSubmitRequest should zero the request if it fails.
But the call to StorePrepareRequest() in StoreTransactionEnd() comes 
after this check:
1560    status = STATUS_RETRY;
1561    if (!Transaction->Active)
1562        goto done;

so it shouldn't fail. The problem, I suspect, is that there's a race. 
The above check is done under Context->Lock but the test inside 
StorePrepareRequest() is not. So, I guess we need to wrap all calls to 
StorePrepareRequest() in the Context->Lock and avoid taking it internally.
I can come up with a suitable patch.

  Paul

> Signed-off-by: Owen Smith <owen.smith@xxxxxxxxxx>
> ---
>   src/xenbus/store.c | 13 +++++++++----
>   1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/src/xenbus/store.c b/src/xenbus/store.c
> index 5ffea1f..778414c 100644
> --- a/src/xenbus/store.c
> +++ b/src/xenbus/store.c
> @@ -966,6 +966,8 @@ StoreSubmitRequest(
>   fail1:
>       Error("fail1 (%08x)\n", status);
>   
> +    RtlZeroMemory(Request, sizeof (XENBUS_STORE_REQUEST));
> +
>       return NULL;
>   }
>   
> @@ -1571,17 +1573,18 @@ StoreTransactionEnd(
>                                    XS_TRANSACTION_END,
>                                    (Commit) ? "T" : "F", 2,
>                                    NULL, 0);
> -    ASSERT(NT_SUCCESS(status));
> +    if (!NT_SUCCESS(status))
> +        goto fail1;
>   
>       Response = StoreSubmitRequest(Context, &Request);
>   
>       status = STATUS_NO_MEMORY;
>       if (Response == NULL)
> -        goto fail1;
> +        goto fail2;
>   
>       status = StoreCheckResponse(Response);
>       if (!NT_SUCCESS(status) && status != STATUS_RETRY)
> -        goto fail2;
> +        goto fail3;
>   
>       StoreFreeResponse(Response);
>       ASSERT(IsZeroMemory(&Request, sizeof (XENBUS_STORE_REQUEST)));
> @@ -1605,11 +1608,13 @@ done:
>   
>       return status;
>   
> -fail2:
> +fail3:
>       ASSERT3U(status, !=, STATUS_RETRY);
>   
>       StoreFreeResponse(Response);
>   
> +fail2:
> +
>   fail1:
>       ASSERT(IsZeroMemory(&Request, sizeof (XENBUS_STORE_REQUEST)));
>