[PATCH v3 1/8] server: Actually set initial status in set_async_direct_result handler.

[Zebediah Figura (she/her)]

On 2/21/22 11:23, Jinoh Kang wrote:
> Commit 15483b1a126 (server: Allow calling async_handoff() with status
> code STATUS_ALERTED., 2022-02-10) introduced the set_async_direct_result
> handler which calls async_set_initial_status().
> 
> However, the async_set_initial_status() call does nothing since
> async->terminated is set, leaving the async in a confusing state
> (unknown_status = 1 but pending/completed).
> 
> So far, this issue is unlikely to have been a problem in practice for
> the following reasons:
> 
> 1. async_set_initial_status() would have unset unknown_status, but it
>    remains set instead.  This is usually not a problem, since
>    unknown_status is usually ever read by code paths effectively
>    unreachable for non-device (e.g. socket) asyncs.
> 
>    It would still potentially allow set_async_direct_result to be called
>    multiple times, but it wouldn't actually happen in practice unless
>    something goes wrong.
> 
> 2. async_set_initial_status() would have set initial_status; however,
>    it is left with the default value STATUS_PENDING.  If the actual
>    status is something other than that, the handler closes the wait
>    handle and async_satisfied (the only realconsumer of initial_status)
>    would never be called anyway.
> 
> For reasons above, this issue is not effectively observable or testable.
> Nonetheless, the current code does leave the async object in an
> inconsistent state.
> 
> Fix this by setting the initial_status directly (and unsetting
> unknown_status as well).
> 
> Signed-off-by: Jinoh Kang <jinoh.kang.kr at gmail.com>
> ---
> 
> Notes:
>     v1 -> v2: no changes
>     v2 -> v3: no changes
> 
>  server/async.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/server/async.c b/server/async.c
> index 7d0ff10f7a4..f078f42f34c 100644
> --- a/server/async.c
> +++ b/server/async.c
> @@ -769,7 +769,8 @@ DECL_HANDLER(set_async_direct_result)
>          return;
>      }
>  
> -    async_set_initial_status( async, status );
> +    async->initial_status = status;
> +    async->unknown_status = 0;
>  
>      if (status == STATUS_PENDING)
>      {

I feel like this raises further questions, like "why does
async_set_initial_status() assert async->unknown_status but then return
if async->terminated?"

For IRPs I think the async can be terminated with unknown_status if the
process dies. I'm not sure if we have an actual bug here, but
conceptually we should probably be resetting unknown_status and setting
initial_status in async_set_result() if necessary.

Of course, if we do that, I think it becomes possible to call
async_set_initial_status() on an async which already has
!async->unknown_status, so that assert() would have to go away.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 495 bytes
Desc: OpenPGP digital signature
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20220225/99ef5089/attachment.sig>