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

[Zebediah Figura (she/her)]

On 3/1/22 07:23, Jinoh Kang wrote:
> On 2/26/22 03:57, Zebediah Figura (she/her) wrote:
>> 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
> 
> To prevent overwriting initial status that was already set, I guess?
> 
>> but then return if async->terminated?"
> 
> This function was introduced in commit 484b78bda01 (ntoskrnl: Report the initial status of an IRP separately from the IOSB status., 2021-09-12), but it provides no explanation.
> 
> I guess it was to keep it in sync with set_async_pending()?

Broadly, or, put another way, I think the answer is "the code was 
already like that" :-)

> That said, I suppose nothing will go wrong even if we pull out the !async->terminated check.
> Nobody really cares about the initial status after termination, except async_satisfied().

Right, and anyway I don't think there's any reason that we should care here.

>> 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,
> 
> This is not a bug (yet), but subsequent patches in this series require that the initial status is eventually set.
> 
>> but conceptually we should probably be resetting unknown_status and setting
>> initial_status in async_set_result() if necessary.
> 
> Great.
> 
>>
>> 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.
> 
> Yeah, and also the !async->terminated check.

Right, that's my thinking too :-)