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

[parser] MemoryCopy() calls: Prevent buffer overflow by replacing har… #4011

Merged
merged 1 commit into from
May 29, 2024

Conversation

avx0
Copy link
Contributor

@avx0 avx0 commented May 29, 2024

…d-coded arguments

In future, if a dev edits the second arg and miscalulates the corresponding 3rd arg, there will be a buffer overflow or the string (2nd arg) will be cut short. This commit prevents that.

…d-coded arguments

In future, if a dev edits the second arg and miscalulates the corresponding 3rd arg, there will be a buffer overflow or the string (2nd arg) will be cut short. This commit prevents that.
@avx0
Copy link
Contributor Author

avx0 commented May 29, 2024

MemoryCopy() does not do input validation on its args.

  1. Is count == size(srcPtr)?
    I wrote some code to set count equal to size(srcPtr), ignoring the user's input of count (this prevents out-of-bounds write / buffer overflow if user inputs count > size(srcPtr). I don't know the reliability of my size() function, please test it and suggest modifications.
// my edit -- begin
int size(char *a)
{
     int sz = 0;
     if (sizeof(a) != 8) return sizeof(a);
     while (a[sz] != '\0') sz++;
     return sz;
}
// my edit -- end

static void MemoryCopy(void *dest, const void *src, unsigned int count)
{
     char *srcPtr = (char *)src;
     char *destPtr = (char *)dest;

// my edit -- begin
     count = size(srcPtr);
// my edit -- end

// out-of-bounds write
     for (unsigned int i = 0; i < count; i++) destPtr[i] = srcPtr[i];
}
  1. size(destPtr) >? size(srcPtr)
    This issue still remains. There is no reliable way of checking it.

@raysan5 raysan5 merged commit 9cc7e35 into raysan5:master May 29, 2024
@raysan5
Copy link
Owner

raysan5 commented May 29, 2024

@avx0 I'm merging this change but note that I usually expect the user to provide reliable data, I can't consider all the breaking situations resulting from a missuse of the library, it only adds code complexity and maintenance burden, and only to cover a very minimum number of use-case situations that hardly ever will happen.

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