Replace malloc's with Godot's memalloc macro #46900
Conversation
akien-mga
added
the
cherrypick:3.x
|
Shouldn't you use Edit: @reduz confirmed that the matching |
| @@ -636,7 +635,8 @@ GD_PINVOKE_EXPORT int32_t _monodroid_get_dns_servers(void **r_dns_servers_array) | |||
|
|
|||
| if (dns_servers_count > 0) { | |||
| size_t ret_size = sizeof(char *) * (size_t)dns_servers_count; | |||
| *r_dns_servers_array = malloc(ret_size); // freed by the BCL | |||
| *r_dns_servers_array = memalloc(ret_size); // freed by the BCL | |||
| ERR_FAIL_NULL_MSG(r_dns_servers_array, "Out of memory."); | |||
There was a problem hiding this comment.
Like the comment says, this is freed by the Mono BCL which doesn't use Godot's memfree (Memory::free_static). Isn't that going to cause trouble?
Honestly, I would prefer this file wasn't touched. Mono's allocations don't use Godot's allocation macros any way.
There was a problem hiding this comment.
See #46807 - if malloc fails (e.g. OOM), this can lead to a null pointer dereference.
But yes in this file we can likely add ERR_FAIL_NULL_MSG on the result of malloc() without changing it to Godot's static alloc.
rsubtil
force-pushed
the
bugfix-malloc_calls
branch
from
1f1fcf7 to
05f3a47
Compare
|
Quick summary of the changes I've made:
Other than these, I've changed the corresponding free's to Godot's macro. I was also wrongly testing some allocations (testing (I am depending on the CI to know if this compiles to the many platforms this affects, so I might need to do quite some force-pushes) |
rsubtil
force-pushed
the
bugfix-malloc_calls
branch
from
05f3a47 to
91dc2f2
Compare
| *r_vertices = (int *)memalloc(sizeof(int) * output.vertexCount); | ||
| ERR_FAIL_NULL_V_MSG(*r_vertices, false, "Out of memory."); | ||
| *r_uvs = (float *)memalloc(sizeof(float) * output.vertexCount * 2); | ||
| ERR_FAIL_NULL_V_MSG(*r_uvs, false, "Out of memory."); | ||
| *r_indices = (int *)memalloc(sizeof(int) * output.indexCount); | ||
| ERR_FAIL_NULL_V_MSG(*r_indices, false, "Out of memory."); |
There was a problem hiding this comment.
These are members of xatlas::Mesh which is in thirdparty code and doesn't use memfree(), so I'm not sure they should be changed to memalloc(). CC @reduz
There was a problem hiding this comment.
Reverted to malloc, and reverted to normal free's in scene/resources/mesh.cpp
There was a problem hiding this comment.
I think there was confusion here, xatlas::Mesh is not related to scene/resources/mesh.cpp. xatlas is a thirdparty library in thirdparty/xatlas/, and it has its own Mesh struct.
For Godot's Mesh, whether or not those values should be memfreed depends on how they're allocated initially. I did see some memalloc related to that code but I'm not so confident to say if the change should be kept.
| *r_value = (char *)malloc(len + 1); | ||
| if (!*r_value) | ||
| return -1; | ||
| *r_value = (char *)memalloc(len + 1); |
There was a problem hiding this comment.
This is freed at line 629 and should be changed to memfree() - or kept as malloc() here if @neikeq thinks it's better.
There was a problem hiding this comment.
Nice catch 🙂. I couldn't find any call to this method, maybe VSCode is skipping files that are ignored due to #if guards, I'll have to see if I can fix this.
I agree to use malloc here again; honestly, I'm scared of the scope this PR has, so maybe we should just stick with malloc where there are worries it might break something. The objective is to prevent null de-references from happening anyways.
There was a problem hiding this comment.
I prefer to keep it as it was as I'm not 100% sure it's always freed with the monodroid_free we implement above. I prefer not to risk.
rsubtil
force-pushed
the
bugfix-malloc_calls
branch
from
91dc2f2 to
838e7d0
Compare
|
Thanks! |
|
Cherry-picked for 3.2.4. |
akien-mga
removed
the
cherrypick:3.x
(cherry picked from commit 2274d4e)
(cherry picked from commit 2274d4e)
Replaces some "raw" calls to
mallocand handle error scenarios.Fixes #46807