Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIX: Resolve stack overflow caused by insufficient disk space. #479

Merged
merged 5 commits into from
May 21, 2024

Conversation

xuezhulian
Copy link
Contributor

When the disk space is insufficient, the ksfu_flushBufferedWriter method will return false, and the value of writer->position will not be set to 0. Continuing to write values to writer->buffer at this point will result in a stack overflow.

Copy link
Collaborator

@GLinnik21 GLinnik21 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix! Just to clarify, did you mean buffer overflow instead of stack overflow? The issue seems related to the buffer exceeding its capacity if ksfu_flushBufferedWriter fails.

Source/KSCrash/Recording/Tools/KSFileUtils.c Outdated Show resolved Hide resolved
Source/KSCrash/Recording/Tools/KSFileUtils.c Outdated Show resolved Hide resolved
@xuezhulian
Copy link
Contributor Author

@GLinnik21 writer->buffer is a variable allocated on the stack, so it causes a stack overflow.
char writeBuffer[1024];

@GLinnik21
Copy link
Collaborator

Let’s add some unit tests. If it’s too complex without mocks or dependency injection, we can merge it as is.

@xuezhulian
Copy link
Contributor Author

Sorry, writing unit tests is quite challenging for me.😞

@GLinnik21 GLinnik21 merged commit 0f5e492 into kstenerud:master May 21, 2024
19 checks passed
bamx23 pushed a commit that referenced this pull request May 24, 2024
* FIX: Resolve stack overflow caused by insufficient disk space.

* modify the format

* FIX: Resolve stack overflow caused by insufficient disk space.

* modify the format

---------

Co-authored-by: yuencong <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants