Change: [Win32] don't allocate 192KiB of memory on the stack on crash #11240
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
TrueBrain
changed the title
TrueBrain
commented
Aug 27, 2023
| char32_t c; | ||
| while ((c = Utf8Consume(&unix_nl)) && p < lastof(dos_nl) - 4) { // 4 is max number of bytes per character | ||
| while ((c = Utf8Consume(&crashlog_unix_nl))) { |
There was a problem hiding this comment.
The removal of the p < check is safe, as crashlog_dos_nl is calculated to be at least as wide as crashlog_unix_nl. So it is not possible for the latter to return a character while the first can't fit it.
(please confirm my claim, ofc :D )
There was a problem hiding this comment.
And \n are counted twice to make room for \r in crashlog_dos_nl. I think I can confirm your claim :).
TrueBrain
force-pushed
the
crashlog-windows-stack
branch
from
d08b905
to
e41978b
Compare
glx22
reviewed
Aug 27, 2023
Heap is out of the question, as it might be corrupted. Allocating this much on stack is silly. So instead, allocate virtual pages to write the information in.
TrueBrain
force-pushed
the
crashlog-windows-stack
branch
from
e41978b
to
f478089
Compare
glx22
approved these changes
Aug 28, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation / Problem
While looking at the Windows crashlog dialog, I noticed it was allocating a lot of memory on the stack. So I looked at JGRPP, and got some inspiration how to solve it properly.
Description
Heap is out of the question, as it might be corrupted. Allocating this much on stack is silly.
So instead, allocate virtual pages to write the information in.
While at it, also added the savegame filename to the dialog, and as they are (now) always set, remove any conditional around them. This simplifies the code a lot, as now it can be printed by a single snprint.
Limitations
Where JGRPP took the approach of: better allocate too much memory than run out of a buffer, I went for: let's do the math correctly, and make sure all buffers fit, while making them nice and snugly. This means the buffers are on the byte of the correct size.
JGRPP's approach might be more robust; although I am pretty sure my math adds up, and should be safe. I hope.
Checklist for review
Some things are not automated, and forgotten often. This list is a reminder for the reviewers.