Jinoh Kang (@iamahuman) commented about dlls/win32u/winstation.c:
+/* return a locked session object for a thread id and type */ +static BOOL get_thread_session_object( UINT tid, enum object_type type, struct object_info *info,
struct object_lock *lock )+{
- struct shared_session *session;
- BOOL valid = TRUE;
- TRACE( "tid %04x, type %u\n", tid, type );
- memset( info, 0, sizeof(*info) );
- info->index = -1;
- while ((session = get_shared_session( !valid )))
- {
info->session_id = session->id;if ((info->index = get_thread_session_object_index( tid, type, &info->id )) == -1) break;
Suppose that:
1. `get_thread_session_object_index` succeeded in the previous iteration, but 2. `get_thread_session_object_index` failed in the current iteration.
Then, `info->id` has been set to nonzero value due to (1), but `info->index` is set to `-1` due to (2).
On the next `get_shared_desktop()` call, `try_lock_session_object()` will notice the nonzero id, and take the address of `objects[-1]` which is out-of-bounds.
---
You have two ways to address this, depending on how you want to treat the `info` parameter:
| Postcondition on failure | Changes to callee (`get_thread_session_object`) | Changes to caller (`get_shared_desktop`) | |--------------------------|----------------------------------------|----------| | `info` is clobbered | (No changes) | **Don't pass a persistent structure via the `info` parameter.** | | `info` is zeroed | **Zero `info` before returning `FALSE`.** | (No changes) |
In general, if you want to handle errors gracefully, you have to do it seriously or not at all. It's essential to define a clear code contract[^1][^2] even if you're not putting it into words. If this seems like too much work, you should just crash on failure instead of introducing subtle failure modes.
[^1]: [Design by contract - Wikipedia](https://en.wikipedia.org/wiki/Design_by_contract) [^2]: C# example: [Code Contracts - .NET Framework | Microsoft Learn](https://learn.microsoft.com/en-us/dotnet/framework/debug-trace-profile/code-contracts)