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

[Jinoh Kang]

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()?

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().

> 
> 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.


-- 
Sincerely,
Jinoh Kang