fix Heart (libthecore) memleak correction

[Thorek 374]

Hi, what's the problem?

Heart uses LPHEART typedef which is a pointer to a structure. Let's take a look at the heart_new function:

As you can see, this function calls calloc to allocate memory for LPHEART,

but this operation causes a little issue - calloc is never released!

- Wait... we have function to free memory (heart_delete)

- Yes, never used anywhere...

My potential solution

Instead of calling heart_delete, I use a smart pointer to store LPHEART

which will be automatically freed.

libthecore -> heart.h:

// We need to move the raw pointer to shared:
typedef struct std::shared_ptr<HEART> LPHEART;

libthecore -> heart.cpp:

// From heart_new function remove this variable:
LPHEART ht;

// Then edit:
CREATE(ht, HEART, 1);

// In this way:
auto ht = std::make_shared<HEART>();

Of course you can remove the heart_delete function from both files now. That's all!

Edited July 29, 2024 by Metin2 Dev International
Core X - External 2 Internal

[Kaptan Yosun 859]

Smart solution, respects to you for sharing it. Much appreciated.

[saudidos 14]

hello great job but u forget to add 

#include <memory> 

in header of  heart.h:

 

#ifndef __INC_LIBTHECORE_HEART_H__
#define __INC_LIBTHECORE_HEART_H__

#include <memory> 

struct HEART; 

using LPHEART = std::shared_ptr<HEART>; 

typedef void (*HEARTFUNC) (LPHEART heart, int pulse);

struct HEART
{
    HEARTFUNC func;

    struct timeval before_sleep;
    struct timeval opt_time;
    struct timeval last_time;

    int passes_per_sec;
    int pulse;
};

// Function declarations
extern LPHEART heart_new(int opt_usec, HEARTFUNC func);
extern int heart_idle(LPHEART ht); // ¸î pulse°¡ Áö³µ³ª ¸®ÅÏÇÑ´Ù.
extern void heart_beat(LPHEART ht, int pulses);

#endif // __INC_LIBTHECORE_HEART_H__

 

[Endymion 399]

Can you explain this memory leak to me?

[Thorek 374]

1 hour ago, Endymion said:

Can you explain this memory leak to me?

 

 

Spoiler

Edited July 29, 2024 by Metin2 Dev International
Core X - External 2 Internal

[Endymion 399]

Can you seriously explain to me, without memes, what this changes? 

Edited July 19, 2024 by Endymion

[Thorek 374]

9 minutes ago, Endymion said:

Can you seriously explain to me, without memes, what this changes? 

All data allocated by malloc/calloc should be freed using free, otherwise it will result in a memory leak:

When you free memory:

Of course my example caused a huge leak by using an infinite loop.

King regards

Edited July 29, 2024 by Metin2 Dev International
Core X - External 2 Internal

[Endymion 399]

There is a difference between what you are modifying in the topic and your example screenshots.

The function you are editing is only called once at the start of the process and never again, so how would it have any effect and make any real difference? We only allocate memory once at process startup, we don't have any loop that does this several times without clearing it.

[Thorek 374]

10 minutes ago, Endymion said:

There is a difference between what you are modifying in the topic and your example screenshots.

The function you are editing is only called once at the start of the process and never again, so how would it have any effect and make any real difference? We only allocate memory once at process startup, we don't have any loop that does this several times without clearing it.

Yep, but what prevents us from freeing callocated memory (even once) and sticking to bad habits, that are already present in the game anyway?

Edited July 20, 2024 by Thorek

[Endymion 399]

But this still doesn't change anything, shared_ptr in this case will only release the memory anyway when the process closes, just as it will do without any changes. 

[Kaptan Yosun 859]

3 hours ago, Endymion said:

But this still doesn't change anything, shared_ptr in this case will only release the memory anyway when the process closes, just as it will do without any changes. 

This is the mentality of a programmer from 50 years ago.

[Thorek 374]

13 hours ago, Endymion said:

But this still doesn't change anything, shared_ptr in this case will only release the memory anyway when the process closes, just as it will do without any changes. 

If you like information about leaks in your programs, you can say so, but facts cannot be deceived.

Edited July 29, 2024 by Metin2 Dev International
Core X - External 2 Internal

[martysama0134 10731]

I made it via std::unique_ptr.

Full Patch:

diff --git a/asf_server/Srcs/Server/db/src/Main.cpp b/asf_server/Srcs/Server/db/src/Main.cpp
index a34e7763..617040d5 100644
--- a/asf_server/Srcs/Server/db/src/Main.cpp
+++ b/asf_server/Srcs/Server/db/src/Main.cpp
@@ -105,7 +105,7 @@ int main()
 	return 1;
 }
 
-void emptybeat(LPHEART heart, int pulse)
+void emptybeat(LPHEART & heart, int pulse)
 {
 	if (!(pulse % heart->passes_per_sec))
 	{
diff --git a/asf_server/Srcs/Server/game/src/main.cpp b/asf_server/Srcs/Server/game/src/main.cpp
index 15f06e3d..5f6cfd69 100644
--- a/asf_server/Srcs/Server/game/src/main.cpp
+++ b/asf_server/Srcs/Server/game/src/main.cpp
@@ -208,7 +208,7 @@ namespace
 extern std::vector<TPlayerTable> g_vec_save;
 unsigned int save_idx = 0;
 
-void heartbeat(LPHEART ht, int pulse)
+void heartbeat(LPHEART & ht, int pulse)
 {
 	DWORD t;
 
diff --git a/asf_server/Srcs/Server/libthecore/include/heart.h b/asf_server/Srcs/Server/libthecore/include/heart.h
index 45fb20f9..2feb6443 100644
--- a/asf_server/Srcs/Server/libthecore/include/heart.h
+++ b/asf_server/Srcs/Server/libthecore/include/heart.h
@@ -1,10 +1,11 @@
 #ifndef __INC_LIBTHECORE_HEART_H__
 #define __INC_LIBTHECORE_HEART_H__
 
+#include <memory>
 typedef struct heart	HEART;
-typedef struct heart *	LPHEART;
+using LPHEART = std::unique_ptr<heart>;
 
-typedef void (*HEARTFUNC) (LPHEART heart, int pulse);
+typedef void (*HEARTFUNC) (LPHEART & heart, int pulse);
 
 struct heart
 {
@@ -19,8 +20,8 @@ struct heart
 };
 
 extern LPHEART	heart_new(int opt_usec, HEARTFUNC func);
-extern void	heart_delete(LPHEART ht);
-extern int	heart_idle(LPHEART ht);
-extern void	heart_beat(LPHEART ht, int pulses);
+extern void	heart_delete(LPHEART & ht);
+extern int	heart_idle(LPHEART & ht);
+extern void	heart_beat(LPHEART & ht, int pulses);
 
 #endif
diff --git a/asf_server/Srcs/Server/libthecore/src/heart.c b/asf_server/Srcs/Server/libthecore/src/heart.c
index 7316972b..13a36267 100644
--- a/asf_server/Srcs/Server/libthecore/src/heart.c
+++ b/asf_server/Srcs/Server/libthecore/src/heart.c
@@ -6,16 +6,13 @@ extern volatile int num_events_called;
 
 LPHEART heart_new(int opt_usec, HEARTFUNC func)
 {
-    LPHEART ht;
-
     if (!func)
     {
 	sys_err("no function defined");
 	return nullptr;
     }
 
-    CREATE(ht, HEART, 1);
-
+    auto ht = std::make_unique<heart>();
     ht->func = func;
     ht->opt_time.tv_sec = 0;
     ht->opt_time.tv_usec = opt_usec;
@@ -27,10 +24,10 @@ LPHEART heart_new(int opt_usec, HEARTFUNC func)
 
 void heart_delete(LPHEART ht)
 {
-    free(ht);
+    ht.reset();
 }
 
-int heart_idle(LPHEART ht)
+int heart_idle(LPHEART & ht)
 {
     struct timeval now, process_time, timeout, temp_time;
     int missed_pulse;

 

Edited July 28, 2024 by martysama0134

[Kuqsal 241]

On 7/28/2024 at 10:17 PM, martysama0134 said:

I made it via std::unique_ptr.

Full Patch:

diff --git a/asf_server/Srcs/Server/db/src/Main.cpp b/asf_server/Srcs/Server/db/src/Main.cpp
index a34e7763..617040d5 100644
--- a/asf_server/Srcs/Server/db/src/Main.cpp
+++ b/asf_server/Srcs/Server/db/src/Main.cpp
@@ -105,7 +105,7 @@ int main()
 	return 1;
 }
 
-void emptybeat(LPHEART heart, int pulse)
+void emptybeat(LPHEART & heart, int pulse)
 {
 	if (!(pulse % heart->passes_per_sec))
 	{
diff --git a/asf_server/Srcs/Server/game/src/main.cpp b/asf_server/Srcs/Server/game/src/main.cpp
index 15f06e3d..5f6cfd69 100644
--- a/asf_server/Srcs/Server/game/src/main.cpp
+++ b/asf_server/Srcs/Server/game/src/main.cpp
@@ -208,7 +208,7 @@ namespace
 extern std::vector<TPlayerTable> g_vec_save;
 unsigned int save_idx = 0;
 
-void heartbeat(LPHEART ht, int pulse)
+void heartbeat(LPHEART & ht, int pulse)
 {
 	DWORD t;
 
diff --git a/asf_server/Srcs/Server/libthecore/include/heart.h b/asf_server/Srcs/Server/libthecore/include/heart.h
index 45fb20f9..2feb6443 100644
--- a/asf_server/Srcs/Server/libthecore/include/heart.h
+++ b/asf_server/Srcs/Server/libthecore/include/heart.h
@@ -1,10 +1,11 @@
 #ifndef __INC_LIBTHECORE_HEART_H__
 #define __INC_LIBTHECORE_HEART_H__
 
+#include <memory>
 typedef struct heart	HEART;
-typedef struct heart *	LPHEART;
+using LPHEART = std::unique_ptr<heart>;
 
-typedef void (*HEARTFUNC) (LPHEART heart, int pulse);
+typedef void (*HEARTFUNC) (LPHEART & heart, int pulse);
 
 struct heart
 {
@@ -19,8 +20,8 @@ struct heart
 };
 
 extern LPHEART	heart_new(int opt_usec, HEARTFUNC func);
-extern void	heart_delete(LPHEART ht);
-extern int	heart_idle(LPHEART ht);
-extern void	heart_beat(LPHEART ht, int pulses);
+extern void	heart_delete(LPHEART & ht);
+extern int	heart_idle(LPHEART & ht);
+extern void	heart_beat(LPHEART & ht, int pulses);
 
 #endif
diff --git a/asf_server/Srcs/Server/libthecore/src/heart.c b/asf_server/Srcs/Server/libthecore/src/heart.c
index 7316972b..13a36267 100644
--- a/asf_server/Srcs/Server/libthecore/src/heart.c
+++ b/asf_server/Srcs/Server/libthecore/src/heart.c
@@ -6,16 +6,13 @@ extern volatile int num_events_called;
 
 LPHEART heart_new(int opt_usec, HEARTFUNC func)
 {
-    LPHEART ht;
-
     if (!func)
     {
 	sys_err("no function defined");
 	return nullptr;
     }
 
-    CREATE(ht, HEART, 1);
-
+    auto ht = std::make_unique<heart>();
     ht->func = func;
     ht->opt_time.tv_sec = 0;
     ht->opt_time.tv_usec = opt_usec;
@@ -27,10 +24,10 @@ LPHEART heart_new(int opt_usec, HEARTFUNC func)
 
 void heart_delete(LPHEART ht)
 {
-    free(ht);
+    ht.reset();
 }
 
-int heart_idle(LPHEART ht)
+int heart_idle(LPHEART & ht)
 {
     struct timeval now, process_time, timeout, temp_time;
     int missed_pulse;

 

Heart.h
 

#ifndef __INC_LIBTHECORE_HEART_H__
#define __INC_LIBTHECORE_HEART_H__

#define MEMORY_LEAK_LIBTHECORE

#ifdef MEMORY_LEAK_LIBTHECORE
#include<memory>
#endif
typedef struct heart	HEART;

#ifndef MEMORY_LEAK_LIBTHECORE
typedef struct heart *	LPHEART;
#else
using LPHEART = std::unique_ptr<heart>;
#endif

#ifndef MEMORY_LEAK_LIBTHECORE
typedef void (*HEARTFUNC) (LPHEART heart, int pulse);
#else
typedef void (*HEARTFUNC) (LPHEART & heart, int pulse);
#endif

struct heart
{
    HEARTFUNC		func;

    struct timeval	before_sleep;
    struct timeval	opt_time;
    struct timeval	last_time;

    int			passes_per_sec;
    int			pulse;
};

extern LPHEART	heart_new(int opt_usec, HEARTFUNC func);

#ifndef MEMORY_LEAK_LIBTHECORE
extern void	heart_delete(LPHEART ht);
extern int	heart_idle(LPHEART ht);	// 몇 pulse가 지났나 리턴한다.
extern void	heart_beat(LPHEART ht, int pulses);
#else
extern void	heart_delete(LPHEART & ht);
extern int	heart_idle(LPHEART & ht);
extern void	heart_beat(LPHEART & ht, int pulses);
#endif

#endif

Heart.c

/*
 *    Filename: heart.c
 * Description: fps 에 한번씩 호출되는 "심장" 이다.
 *
 *      Author: 비엽 aka. Cronan
 */
#define __LIBTHECORE__
#include "stdafx.h"

#define MEMORY_LEAK_LIBTHECORE

extern volatile int num_events_called;

LPHEART heart_new(int opt_usec, HEARTFUNC func)
{
#ifndef MEMORY_LEAK_LIBTHECORE
    LPHEART ht;
#endif

    if (!func)
    {
	sys_err("no function defined");
	return NULL;
    }

#ifndef MEMORY_LEAK_LIBTHECORE
    CREATE(ht, HEART, 1);
#endif

#ifdef MEMORY_LEAK_LIBTHECORE
	auto ht = std::unique_ptr<heart>();
#endif
    ht->func = func;
    ht->opt_time.tv_sec = 0;
    ht->opt_time.tv_usec = opt_usec;
    ht->passes_per_sec = 1000000 / opt_usec;
    gettimeofday(&ht->last_time, (struct timezone *) 0);
    gettimeofday(&ht->before_sleep, (struct timezone *) 0);
    return (ht);
}

void heart_delete(LPHEART ht)
{
#ifndef MEMORY_LEAK_LIBTHECORE
    free(ht);
#else
	ht.reset();
#endif

}

#ifndef MEMORY_LEAK_LIBTHECORE
int heart_idle(LPHEART ht)
#else
	int heart_idle(LPHEART & ht)
#endif
{
    struct timeval now, process_time, timeout, temp_time;
    int missed_pulse;

    gettimeofday(&ht->before_sleep, (struct timezone *) 0);
    process_time = *timediff(&ht->before_sleep, &ht->last_time);

    /*
     * If we were asleep for more than one pass, count missed pulses and sleep
     * until we're resynchronized with the next upcoming pulse.
     */
    if (process_time.tv_sec == 0 && process_time.tv_usec < ht->opt_time.tv_usec)
    {
	missed_pulse = 0;
    }
    else
    {
	missed_pulse = process_time.tv_sec * ht->passes_per_sec;
	missed_pulse += process_time.tv_usec / ht->opt_time.tv_usec;
    }

	// 바빠서 pulse도 놓쳤는데 잘 시간이 어딨어...
	// 펄스 fps 어차피 틀어져있는데, 정확히 맞추는 건 중요하지 않아.
	if (missed_pulse > 0)
	{
		gettimeofday(&ht->last_time, (struct timezone *) 0);
	}
	else
	{
		/* Calculate the time we should wake up */
		temp_time = *timediff(&ht->opt_time, &process_time);
		ht->last_time = *timeadd(&ht->before_sleep, &temp_time);

		/* Now keep sleeping until that time has come */
		gettimeofday(&now, (struct timezone *) 0);
		timeout = *timediff(&ht->last_time, &now);

		thecore_sleep(&timeout);
	}

    ++missed_pulse;

    if (missed_pulse <= 0)
    {
	sys_err("missed_pulse is not positive! (%d)", missed_pulse);
	missed_pulse = 1;
    }

    if (missed_pulse > (30 * ht->passes_per_sec))
    {
	sys_err("losing %d seconds. (lag occured)", missed_pulse / ht->passes_per_sec);
	missed_pulse = 30 * ht->passes_per_sec;
    }

    return missed_pulse;
}

For example

[Agares 1]

Quote

This situation with LPHEART in the code does not cause any problems, nothing serious. The application is called 1 time when it is first opened, this does not cause memory bloat. This can be done for a cleaner safe code. The safest way is to call heart_delete at the appropriate place when closing the application. It is always better to use raw pointers where high performance is needed, but using smart pointers here will not cause problems.   

 

[Denizeri24 43]

Edited August 13, 2024 by Metin2 Dev International
Core X - External 2 Internal