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 Instances of Zero Sized Memory Allocations #3936

Merged
merged 2 commits into from Jun 4, 2022

Conversation

digitall
Copy link
Member

@digitall digitall commented Jun 1, 2022

Apart from generating warnings from GCC when -Walloc-zero is passed, the behaviour when
malloc or new is called with zero sized is implementation dependent and could result in
portability issues. See:
https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html

-Walloc-zero

    Warn about calls to allocation functions decorated with attribute alloc_size that specify zero bytes, including those to the built-in forms of the functions aligned_alloc, alloca, calloc, malloc, and realloc. Because the behavior of these functions when called with a zero size differs among implementations (and in the case of realloc has been deprecated) relying on it may result in subtle portability bugs and should be avoided.

The GLK Queztal code has an instance of this calling malloc and the WINTERMUTE engine has an instance with C++ new operator for an array in a failure case.

This PR fixes these by replacing them with nullptr, but the engine developers should review to check if the code using this will test correctly in these cases.

digitall added 2 commits Jun 1, 2022
The behaviour of malloc and similar functions when called with a zero
sized parameter is implementation dependent resulting in subtle portability
bugs and should be avoided.

This will generate warnings in GCC if -Walloc-zero is passed.
The behaviour of malloc and similar functions when called with a zero
sized parameter is implementation dependent resulting in subtle portability
bugs and should be avoided.

This will generate warnings in GCC if -Walloc-zero is passed.
@bluegr
Copy link
Member

@bluegr bluegr commented Jun 2, 2022

Thanks for spotting this. The wintermute case is quite straightforward and is done when data can't be decompressed, so this should be safe to apply.

I'm not sure about the GLK case: it tries to create a memory stream from a null pointer if there is no data to read. Not sure how that would work at all - perhaps returning a nullptr when the size is 0 makes more sense

@sev- sev- requested a review from dreammaster Jun 3, 2022
@PaulGilbert
Copy link
Contributor

@PaulGilbert PaulGilbert commented Jun 3, 2022

GLK change looks good to me.

@digitall
Copy link
Member Author

@digitall digitall commented Jun 4, 2022

I concur with @bluegr that the WINTERMUTE case should be safe to apply and since @dreammaster has confirmed that the GLK change is good as well, I will merge this now.

@digitall digitall merged commit cd627ad into scummvm:master Jun 4, 2022
8 checks passed
@digitall digitall deleted the bugfix_zero_sized_allocations branch Jun 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants