Make MAX_CONTEXT_SIZE dynamically sized to future-proof it #2666
Comments
|
For the record: the extended context size for both, Skylake and Kaby Lake, is 0x4F7. |
|
err, above size was the result without CONTEXT_XSTATE. The correct value should be 0x6EF and 0x4E3 for a 32-bit process. |
Yes that's a reasonable short-term fix. The current value in the code today for 64-bit MAX_CONTEXT_SIZE is 0x6ef and for 32-bit 0x4e3 -- are you reporting a subset of the context above? |
|
in 7 RC1 I see 0x680 and 0x480, respectively, defined in os_private.h? |
The only concern here is that stack buffers of this size are used in a number of places in the code and DR has a limited stack size. If the MAX_CONTEXT_SIZE gets too much larger they will need to be changed to use the heap. |
Ah ok, I'm looking at the latest, and it was updated in 5eebc95. So perhaps the short-term fix has already been done? |
|
Oh neat, yes those values are the same that I deduced. Are there nightly builds? |
|
no windows builds? |
|
Unfortunately the old Cr nightly builds no longer exist (we should update that wiki page) and noone has set them up with Appveyor yet (#1967) |
|
I can confirm that the context size related bug is fixed in trunk. |
derekbruening
changed the title
|
Got here via #3923. I tried launching drrun and drmemory via windbg from cygwin, from within windbg, loading symbols to find out the value of If you can provide a source code snippet that I can compile with VS 2013 to call RtlGetExtendedContextLength() on my system, than I'd be happy to compile and run it. |
|
On the system that @securitykernel tried the context size seems to be
|
|
@derekbruening Is there any plan to increase the |
|
Happy to take a pull request for either a short-term size increase or long-term switch to dynamic sizing. |
Long-term we need to dynamically size the context and move it to the heap, but for now we increase the hardcoded size to avoid failures. Issue: #2666
|
This trouble is still reproducible, is it possible to workaround somehow on my side? |
You can see all the discussion above about local builds and changing the constant (and longer-term refactoring). It would be awesome if someone with these affected machine environments who can thus test it were able to send a pull request for at least a short-term fix. |
|
For reference, here are the notes I took then I debugged it on our affected machine to get the needed value. Note that it has been some time since I did it, so no guarantee about correctness/completes about them: All done with WinDbg and a cronbuild from https://github.com/DynamoRIO/dynamorio/releases: As the automatic loading of symbols described at https://github.com/DynamoRIO/dynamorio/wiki/Debugging#debugging-on-windows did nor work for use, we had to load the symbols manually (adapt paths below): To get to the nt_initialize_context function (e.g. to get the value on a system that does trigger the assert), execute the following afterwards: Afterwards, make a few manual single steps, until the |
|
@lieser Thank you for your guidance. Sorry for delay. It seems I got the value, but not sure that I do it in the right way, could you please check: |
|
Also not an expert on WinDbg, but as far as I can tell you did everything right. But does this error still happen to you with the latest cronbuild? |
|
Ok, I see, thank you. The difficult is that drmemory crontab releases are appear quite rare: latest - 12 Sep, previous - 4 Jul and 4 Apr |
|
Hello |
|
I think this was accidentally closed: the hardcoded constant was raised, fixing it short-term, but this issue covers a long-term dynamic-sizing fix. Re-opening. |
|
Looks like thread_set_self_mcontext is one place where a heap buffer cannot be freed (at least locally). I think all the others can easily heap-allocate. |
Changes all stack buffers using hardcoded sizes for CONTEXT to instead use heap-allocated buffers sized to match the kernel's actual CONTEXT size for the given flags. Adds a mechanism to query the CONTEXT64 size from 32-bit DR using switch_modes_and_call(). The one place where we can't easily use heap is thread_set_self_mcontext() because it doesn't return and we thus do not have a simple way to free the buffer. There, we use a large 4096-byte stack buffer, and we check the size needed: if it's larger than that, we use a lost heap allocation and issue a debug build warning which should alert someone. If we end up needing more space there in the future we may want to store a pointer in the dcontext and have a special check to free it or something. Fixes #2666
Changes all stack buffers using hardcoded sizes for CONTEXT to instead use heap-allocated buffers sized to match the kernel's actual CONTEXT size for the given flags. Adds a mechanism to query the CONTEXT64 size from 32-bit DR using switch_modes_and_call(). The one place where we can't easily use heap is thread_set_self_mcontext() because it doesn't return and we thus do not have a simple way to free the buffer. There, we use a large 4096-byte stack buffer, and we check the size needed: if it's larger than that, we use a lost heap allocation and issue a debug build warning which should alert someone. If we end up needing more space there in the future we may want to store a pointer in the dcontext and have a special check to free it or something. Fixes #2666
On Skylake this assert fires:
Xref #1937 for the corresponding Linux signal context issue.
We need to make MAX_CONTEXT_SIZE be dynamically sized.
The text was updated successfully, but these errors were encountered: