From 64540bc60d86c9cf48b26fd8b181e1b0a5a6400f Mon Sep 17 00:00:00 2001 From: "Jasper St. Pierre" Date: Sun, 2 Nov 2014 17:32:22 -0800 Subject: [PATCH 1/3] MemArena: Fix a memory leak caused by pointer confusion This code was ported from out_ptr, which was a double-pointer, and wanted to double-check that the proper arena was actually allocated. When I ported it to store the pointer directly in the view regardless of whether out_ptr was non-NULL, I got confused here and instead caused the code to only free the arena if the first byte was non-zero. --- Source/Core/Common/MemArena.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Source/Core/Common/MemArena.cpp b/Source/Core/Common/MemArena.cpp index 60ad058778..119549f06d 100644 --- a/Source/Core/Common/MemArena.cpp +++ b/Source/Core/Common/MemArena.cpp @@ -267,7 +267,7 @@ void MemoryMap_Shutdown(MemoryView *views, int num_views, u32 flags, MemArena *a for (int i = 0; i < num_views; i++) { MemoryView* view = &views[i]; - if (view->mapped_ptr && *(u8*)view->mapped_ptr && !freeset.count(view->mapped_ptr)) + if (view->mapped_ptr && !freeset.count(view->mapped_ptr)) { arena->ReleaseView(view->mapped_ptr, view->size); freeset.insert(view->mapped_ptr); From 5e5ed07b4140b98d2bee9128ad096cfae6c21f8b Mon Sep 17 00:00:00 2001 From: "Jasper St. Pierre" Date: Sun, 2 Nov 2014 19:12:21 -0800 Subject: [PATCH 2/3] MemArena: Fix the calculation of position in SHM The code to calculate the offsets into the SHM file wasn't properly respecting the skip flags, causing it to calculate offsets beyond the end of the SHM file. --- Source/Core/Common/MemArena.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/Source/Core/Common/MemArena.cpp b/Source/Core/Common/MemArena.cpp index 119549f06d..15de0c37b9 100644 --- a/Source/Core/Common/MemArena.cpp +++ b/Source/Core/Common/MemArena.cpp @@ -176,15 +176,17 @@ static bool Memory_TryBase(u8 *base, MemoryView *views, int num_views, u32 flags // We just mimic the popular BAT setup. u32 shm_position = 0; - // Zero all the pointers to be sure. for (int i = 0; i < num_views; i++) { + // Zero all the pointers to be sure. views[i].mapped_ptr = nullptr; - if (!(views[i].flags & MV_MIRROR_PREVIOUS) && i > 0) - shm_position += views[i - 1].size; + SKIP(flags, views[i].flags); views[i].shm_position = shm_position; + + if (!(views[i].flags & MV_MIRROR_PREVIOUS)) + shm_position += views[i].size; } int i; From f66078925456b6d704428f3263f7837100572328 Mon Sep 17 00:00:00 2001 From: "Jasper St. Pierre" Date: Sun, 2 Nov 2014 19:24:21 -0800 Subject: [PATCH 3/3] MemArena: Merge the initialization code with the size calculation code To make mistakes like in the previous commit less prevalent in the future. --- Source/Core/Common/MemArena.cpp | 34 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 18 deletions(-) diff --git a/Source/Core/Common/MemArena.cpp b/Source/Core/Common/MemArena.cpp index 15de0c37b9..a51da9bc3a 100644 --- a/Source/Core/Common/MemArena.cpp +++ b/Source/Core/Common/MemArena.cpp @@ -174,20 +174,6 @@ static bool Memory_TryBase(u8 *base, MemoryView *views, int num_views, u32 flags { // OK, we know where to find free space. Now grab it! // We just mimic the popular BAT setup. - u32 shm_position = 0; - - for (int i = 0; i < num_views; i++) - { - // Zero all the pointers to be sure. - views[i].mapped_ptr = nullptr; - - SKIP(flags, views[i].flags); - - views[i].shm_position = shm_position; - - if (!(views[i].flags & MV_MIRROR_PREVIOUS)) - shm_position += views[i].size; - } int i; for (i = 0; i < num_views; i++) @@ -236,16 +222,28 @@ bail: return false; } -u8 *MemoryMap_Setup(MemoryView *views, int num_views, u32 flags, MemArena *arena) +static void MemoryMap_InitializeViews(MemoryView *views, int num_views, u32 flags) { - u32 total_mem = 0; + u32 shm_position = 0; for (int i = 0; i < num_views; i++) { + // Zero all the pointers to be sure. + views[i].mapped_ptr = nullptr; + SKIP(flags, views[i].flags); - if ((views[i].flags & MV_MIRROR_PREVIOUS) == 0) - total_mem += views[i].size; + + views[i].shm_position = shm_position; + + if (!(views[i].flags & MV_MIRROR_PREVIOUS)) + shm_position += views[i].size; } +} + +u8 *MemoryMap_Setup(MemoryView *views, int num_views, u32 flags, MemArena *arena) +{ + MemoryMap_InitializeViews(views, num_views, flags); + u32 total_mem = views[num_views - 1].shm_position; arena->GrabSHMSegment(total_mem);