Doing malloc(0) or not doing malloc(0)?
#2980
Comments
illwieckz
added
the
T-Question
illwieckz
changed the title
malloc(0) or not doing malloc(0)malloc(0) or not doing malloc(0)?
|
I said SDL in the previous comment, but in fact it's Mesa doing it. But not only Mesa does it, but NaCL does it too: |
|
I did that: diff --git a/src/common/System.cpp b/src/common/System.cpp
index 55cd0a036..8d35d02f6 100644
--- a/src/common/System.cpp
+++ b/src/common/System.cpp
@@ -427,11 +427,20 @@ void GenRandomBytes(void* dest, size_t size)
} // namespace Sys
+#include <cstdio>
+
#ifndef USING_ADDRESS_SANITIZER
// Global operator new/delete override to not throw an exception when out of
// memory. Instead, it is preferable to simply crash with an error.
void* operator new(size_t n)
{
+ if (!n)
+ {
+ Log::Warn("malloc(0) attempted\n");
+ }
+
+ ASSERT(n);
+
void* p = malloc(n);
if (!p)
Sys::Error("Out of memory");And even the LLVM backend of the Mesa radeonsi driver does |
|
When running But if I do that: diff --git a/src/engine/client/hunk_allocator.cpp b/src/engine/client/hunk_allocator.cpp
index d42d85511..6b21ac8e7 100644
--- a/src/engine/client/hunk_allocator.cpp
+++ b/src/engine/client/hunk_allocator.cpp
@@ -222,6 +222,13 @@ Allocate permanent (until the hunk is cleared) memory
*/
void *Hunk_Alloc( int size, ha_pref)
{
+ if (!size)
+ {
+ Log::Warn("Hunk_Alloc(0) attempted\n");
+ }
+
+ ASSERT(size);
+
void *buf;
if ( s_hunkData == nullptr )The first occurrence I catch is the one from the exact same code my PR modifies, but before my PR is even merged!: |
|
OK, this one seems to be legit in startMarker = (byte*) ri.Hunk_Alloc( 0, ha_pref::h_low );🤣️ There is also in s_worldData.dataSize = ( byte * ) ri.Hunk_Alloc( 0, ha_pref::h_low ) - startMarker;🤪️ But this can be a dedicated |
|
I actually have a branch that runs without hitting an The only three files I had to add checks for not calling So this doesn't look very intrusive if we want to go the “never do any kind of The only occurrences of |
|
In general, allocating an array of size zero is perfectly legitimate. This can commonly occur when allocating an array with a size determined at runtime. There is no reason to avoid zero-sized allocations with, e.g., the non-malloc-based "hunk" allocator. The case of which seems good as you only have to check the size in the rarely taken "failure" branch. But we are writing in C++, so we shouldn't be using |
If you allocate a struct and then access the struct members, if the size is 0, you definitely not allocated the struct and the member access will crash. I guess it's just that, why would someone call Even doing zero-size allocation with the ”non-malloc-based "hunk" allocator” looks weird because even if it is assumed it cannot fail (in the meaning the memory is already allocated and if there is a failure it will call If there is nothing to allocate, I would just skip the allocation and the whole related processing code as well. The only three occurrences I spotted of zero-size hunk allocations are in the BSP code (two of them being just hacks to get the current position of the allocatable memory), in the shader code, and in the MD3 model code. BSP, shader and MD3 being basically some of the oldest thing from the engine. There may be other uncaught places, but after I rewrote this 3 use cases in a branch (by just skipping allocation and processing if zero size), I could load a game without facing a zero-sized hunk allocation anymoe. |
I don't see how this would be a common bug as
Because they're allocating an array of length 0.
|
When allocating an empty array |
This topic surfaced multiple times these days.
The stage iteration case
At first when implementing this PR I basically did this:
@slipher made the remark that I could just do:
And it is true that all of this would still work even when
numStagesis equal to0:We can actually run
mallocormemcpywith0size, the code may still work if properly written.The empty vector sending message case
We discovered that with recent Clang, serializing an empty vector when sending a message crashes the game.
It's likely a compiler bug, but we have to deal with it and then we now attempt to prevent creating empty vectors that would be serialized.
Some profilers may assume
malloc(0)is a mistakeThese days I extended my testing build script to support more debuggers and profilers.
I toyed again with Electric Fence, which is a memory profiler that helped me a lot 10 years ago when debugging XQF, so out of curiosity I tried again…
So, I was surprised that by default, Electric Fence stops execution of the instrumented program on
malloc(0)and prints:This check can be intentionnally removed by setting
EF_ALLOW_MALLOC_0=1in environment. But I find interesting that some profilers may assumemalloc(0)is probably a mistake and would be surprising to do on purpose.I guess doing
malloc(0)on purpose makes it harder if not impossible to check for this to happen by mistake in other parts of the code.Others do it anyway
The reason why I noticed Electic Fence catches
malloc(0)as error by default is that because others do it:It first thought it was SDL fault:
But in fact it is happening in Mesa:
So, even if we would make efforts to never do
malloc(0), we would have to disable the check because of others doing it anyway.The question
I don't think it's a bad idea to prevent doing
malloc(0), but I'm not sure we need to enforce that.Even if it may not be incorrect to do it, this may ask for trouble for various reasons. For example if some profilers reports them as potential bugs by default we may assume many developpers avoid to do this, and that would mean doing
malloc(0)would be poorly tested.What is your opinion on this?
The text was updated successfully, but these errors were encountered: