Skip to content

Conversation

@jparr721
Copy link
Contributor

@jparr721 jparr721 commented Nov 17, 2025

These bytes checks can be skipped by putting an integer value for bytes that's large enough to cause the assertion to overflow, resulting in a buffer overflow later in the call. This change always just widens it to make sure the check occurs. Tested on Visual Studio 2022 in Debug and Release.

PoC Reader

#include "serialize.h"
#include <stdio.h>
#include <stdlib.h>
#include <limits.h>

int main()
{
    const int BUFFER_SIZE = 64;
    uint8_t buffer[BUFFER_SIZE];
    memset(buffer, 0xAA, BUFFER_SIZE);

    // Initialize ReadStream with the small buffer
    serialize::ReadStream stream(buffer, BUFFER_SIZE);

    // Attack: INT_MAX causes bytes * 8 to overflow
    const int ATTACK_SIZE = INT_MAX;

    printf("Testing ReadStream with ATTACK_SIZE = %d\n", ATTACK_SIZE);
    printf("Expected: Should not read beyond buffer bounds\n");

    uint8_t output[BUFFER_SIZE];
    stream.SerializeBytes(output, ATTACK_SIZE);

    printf("Test completed\n");
    return 0;
}

PoC Writer

#include "serialize.h"
#include <stdio.h>
#include <stdlib.h>

int main()
{
    const int BUFFER_SIZE = 64;  
    uint8_t buffer[BUFFER_SIZE];
    memset(buffer, 0xAA, BUFFER_SIZE);

    // Initialize WriteStream with the small buffer
    serialize::WriteStream stream(buffer, BUFFER_SIZE);

    const int ATTACK_SIZE = INT_MAX;  

    stream.SerializeBytes(buffer, ATTACK_SIZE);

    return 0;
}

@gafferongames
Copy link
Contributor

If you can fix the warnings on linux I'm willing to consider this PR -- thanks!

@jparr721
Copy link
Contributor Author

If you can fix the warnings on linux I'm willing to consider this PR -- thanks!

Yeah I'll take care of it. I figured I might get burned with the lazy conversion. Thanks for running the CI. I'll copy those commands tomorrow and make sure they work.

@jparr721 jparr721 changed the title [Bug Fix] Fix overflow in failed assert [Bug Fix] Fix buffer overflow as a result of overflowed assert Nov 17, 2025
@jparr721
Copy link
Contributor Author

Hey @gafferongames the build appears to be passing. Let me know if this casting behavior is sufficient. I didn't want to be too heavy handed.

@gafferongames gafferongames merged commit 13bccb4 into mas-bandwidth:main Nov 19, 2025
6 checks passed
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