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

Memory leak in parseRequest #30

Closed
jason-watkins opened this issue Mar 27, 2015 · 2 comments
Closed

Memory leak in parseRequest #30

jason-watkins opened this issue Mar 27, 2015 · 2 comments

Comments

@jason-watkins
Copy link
Contributor

Summary

The parseRequest function creates multiple potential memory leaks.

Description

parseRequestcurrently unconditionally allocates new memory and stores the resulting pointers in resultArray, overwriting any pointers already stored there. This can leak memory in two ways. First, the allocation may overwrite a pointer without freeing it. This was previously handled by freeing non-null pointers in resultArray, but parseRequest is sometimes called with a stack allocated two dimensional array, which causes free to segfault on Windows 8. Second, the responsibility for eventually freeing the newly allocated memory falls to the caller. Since there is no way to enforce this contract, it is inevitable that callers will forget to free memory.

@jason-watkins
Copy link
Contributor Author

I see a few ways to fix this.

  1. Require that the caller pass resultArray with enough memory already allocated. This makes it the explicit responsibility of the caller to handle all memory management, but it opens the door to buffer overflows since there is now way for parseRequest to verify that the size of the arrays stored in resultArray are correct.
  2. Require that the caller pass resultArray with either NULL or valid heap allocated arrays. This allows us to safely call free before re-assigning new allocations to resultArray, but still leave the potential for the caller to forget to free the resulting allocations. It could also be an issue if the caller has another pointer to one of the existing results.
  3. Leave the function as is. Assume the caller knows what they are doing, and let them sort out any memory management issues.

In any of the three cases, the contract for this function probably needs to be stated very explicitly to reduce the likelihood of memory management errors.

@jason-watkins
Copy link
Contributor Author

Latest version of the C client make substantial architectural changes to prevent this kind of issue from cropping up in the future. All client functions now require that memory be fully allocated when passed as an argument, and uses additional parameters to ensure that the arguments are sufficiently large to handle the requested data.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant