fix Fix Great Offshop Memory Leak

[Volvox 1454]

Here is my fix for a huge mem leak on great Offshop.

 

If you use SQLMsg* pkMsg(DBManager::instance().DirectQuery(... :

void CHARACTER::UpdateShopItems()

At the End:
delete Msg;

#######################################################
void CHARACTER::LoadPrivShops()

At the End:
delete pkMsg;

#######################################################
void CHARACTER::OpenShop(DWORD id, const char* name, bool onboot)

At the End:
pkMsg.reset(nullptr);

#######################################################
ACMD(do_shop_update_item)

After:
					if (CHARACTER_MANAGER::instance().GetCharactersByRaceNum(30000, i))
					{
						CharacterVectorInteractor::iterator it = i.begin();
						while (it != i.end())
						{
							LPCHARACTER pc = *it++;
							if (pc)
								if (pc->GetRaceNum() == 30000 && pc->GetPrivShop() == shop_id)
								{
									pc->UpdateShopItems();
									return;
								}
						}
					}
Add:
					delete pkMsg;					

After:
					if (CHARACTER_MANAGER::instance().GetCharactersByRaceNum(30000, i))
					{
						CharacterVectorInteractor::iterator it = i.begin();
						while (it != i.end())
						{
							LPCHARACTER pc = *it++;
							if (pc)
								if (pc->GetRaceNum() == 30000 && pc->GetPrivShop() == shop_id)
								{
									pc->UpdateShopItems();
									return;
								}
						}
					}
Add:
					delete pkMsg;
					
After:
							TPacketShopUpdateItem packet;
							packet.shop_id = shop_id;
							db_clientdesc->DBPacket(HEADER_GD_SHOP_UPDATE_ITEM, 0, &packet, sizeof(packet));
					}
				}
Add:
				delete pkMsg;
					
#######################################################	
ACMD(do_shop_refresh_items)	

At the End:
delete pkMsg;

#######################################################	
EVENTFUNC(fix_shop_event)

Before:
	return PASSES_PER_SEC(SHOP_TIME_REFRESH * 60);
Insert:
	delete pkMsg;

 

Have a look at the names of Msg or pkMsg.

These are just names, so change them, like your function need it !!

 

Edited October 29, 2020 by V0lvox

[JezMan 2]

Thanks V0lvox

[Mali 39462]

you don't have to do for smart ptr

[AKUROS 26]

5 hours ago, Mali61 said:

you don't have to do for smart ptr

why?

[Lufbert 20]

14 minutes ago, AKUROS said:

why?

Because as it names said it's "smart". 

When function execution ends pointer is automatically deallocated and ram is freed (of course only in this simple example with unique_ptr, normally that topic is more complicated for global variables, variables added to vectors etc.).

Edited October 23, 2020 by Lufbert

[AKUROS 26]

21 minutes ago, Lufbert said:

Because as it names said it's "smart". 

When function execution ends pointer is automatically deallocated and ram is freed (of course only in this simple example with unique_ptr, normally that topic is more complicated for global variables, variables added to vectors etc.).

That's right! Thanks

[DeYaN. 27]

Hi there, without thix fix what happen ?

[Volvox 1454]

Memory overflow, because he always allocate mem and never delete the allocation

[Toki.San 1900]

On 10/23/2020 at 1:35 PM, V0lvox said:

#######################################################

void CHARACTER::OpenShop(DWORD id, const char* name, bool onboot)

At the End:
pkMsg.reset(nullptr);
#######################################################

I try This and have this error:

compile char.cpp
char.cpp: In member function 'void CHARACTER::OpenShop(DWORD, const char*, bool)':

char.cpp:8086: error: request for member 'reset' in 'pkMsg', which is of non-class type 'SQLMsg*'
char.cpp:8086: error: 'nullptr' was not declared in this scope

 

Edited October 28, 2020 by TokiSan

[VegaS™ 9939]

Just if you use SQLMsg* pkMsg then it's called a raw pointer and you've to delete it manually, but for std::unique_ptr<SQLMsg>, you don't have to do it, has no effects the reset method in this case.

• https://docs.microsoft.com/en-us/cpp/cpp/raw-pointers?view=vs-2019
• https://docs.microsoft.com/en-us/cpp/cpp/smart-pointers-modern-cpp?view=vs-2019
• https://en.cppreference.com/w/cpp/memory/unique_ptr

The benefit of using smart pointers like unique_prt || shared_ptr is that they automatically delete the pointed object when no longer needed (i.e. out of scope).

This is the hidden content, please

• Sign In
• or
• Sign Up

 

Edited December 3, 2020 by VegaS™

[Volvox 1454]

16 hours ago, TokiSan said:

I try This and have this error:

compile char.cpp
char.cpp: In member function 'void CHARACTER::OpenShop(DWORD, const char*, bool)':

char.cpp:8086: error: request for member 'reset' in 'pkMsg', which is of non-class type 'SQLMsg*'
char.cpp:8086: error: 'nullptr' was not declared in this scope

 

Just use your brain and have a look, what name your variable has.

Mine is "pkMsg", maybe yours is "Msg". 

[DeYaN. 27]

I dont know how the source works , and i need to ask you :

Spoiler

 

i need to delete this all function , or only the "Msg" - from that line ? #same question for pkMsg;

Edited August 25, 2022 by Metin2 Dev
Core X - External 2 Internal

[Volvox 1454]

5 hours ago, DeYaN. said:

I dont know how the source works , and i need to ask you :

  Hide contents

 

i need to delete this all function , or only the "Msg" - from that line ? #same question for pkMsg;

You dont need to delete anything,

Just write at the end of this function:

delete Msg;

Edited August 25, 2022 by Metin2 Dev
Core X - External 2 Internal

[Goof 2]

https://metin2.download/picture/011W569kW3jOYnX4d6d34qVPKg78jce6/.png

mine is pkMsg

 

look https://metin2.download/picture/68nnI330Fjl67N7V9yo5I8FjJh88AHS9/.png

Edited September 4, 2022 by Metin2 Dev
Core X - External 2 Internal

[Ikarus_ 2330]

On 10/28/2020 at 4:55 PM, TokiSan said:

I try This and have this error:

compile char.cpp
char.cpp: In member function 'void CHARACTER::OpenShop(DWORD, const char*, bool)':

char.cpp:8086: error: request for member 'reset' in 'pkMsg', which is of non-class type 'SQLMsg*'
char.cpp:8086: error: 'nullptr' was not declared in this scope

 

 

nullptr is defined since c++11 and i guess you are using reset on a raw pointer (the reset wasn't really needed btw)

Edited November 7, 2020 by Ikarus_

[Toki.San 1900]

8 hours ago, Ikarus_ said:

 

nullptr is defined since c++11 and i guess you are using reset on a raw pointer (the reset wasn't really needed btw)

Umh nope, the problem was msg

Instead of pkmsg. 

[developerares 1]

Hi,

 

    

Quote

std::unique_ptr<SQLMsg> Msg(DBManager::instance().DirectQuery("SELECT id,vnum,count,display_pos,price,price_cheque,transmutation,%s,%s from player_shop_items where shop_id='%d'", szSockets, szAttrs, GetPrivShop()));
    SQLResult * Res = Msg->Get();
    if (Res->uiNumRows > 0)

 

 

Is there a problem with this code? I have smart ptr. 

 

@V0lvox Thank you.

[Volvox 1454]

6 hours ago, developerares said:

Hi,

 

    

 

 

Is there a problem with this code? I have smart ptr. 

 

@V0lvox Thank you.

 

On 10/28/2020 at 5:20 PM, VegaS™ said:

Just if you use SQLMsg* pkMsg then it's called a raw pointer and you've to delete it manually, but for std::unique_ptr<SQLMsg>, you don't have to do it, has no effects the reset method in this case.

• https://docs.microsoft.com/en-us/cpp/cpp/raw-pointers?view=vs-2019
• https://docs.microsoft.com/en-us/cpp/cpp/smart-pointers-modern-cpp?view=vs-2019
• https://en.cppreference.com/w/cpp/memory/unique_ptr

The benefit of using smart pointers like unique_prt || shared_ptr is that they automatically delete the pointed object when no longer needed (i.e. out of scope).

void UseRawPointer()
{
	// Using a raw pointer -- not recommended.
	SQLMsg * pkMsg(DBManager::instance().DirectQuery("SELECT * FROM player.player"));

	// Use pkMsg...
	uint32_t uiNumRows = pkMsg->Get()->uiNumRows;

	// Don't forget to delete!
	delete pkMsg;   
}

void UseSmartPointer()
{
	// Declare a smart pointer on stack and pass it the raw pointer.
	std::unique_ptr<SQLMsg> pkMsg(DBManager::instance().DirectQuery("SELECT * FROM player.player"));

	// Use pkMsg...
	auto uiNumRows = pkMsg->Get()->uiNumRows;
	//...

} // pkMsg is deleted automatically here.

 

 

[Ikarus_ 2330]

On 10/28/2020 at 4:55 PM, TokiSan said:

char.cpp:8086: error: request for member 'reset' in 'pkMsg', which is of non-class type 'SQLMsg*' char.cpp:8086: error: 'nullptr' was not declared in this scope

Uhm, yep, nullptr is not declared means you are not using c++11(+) and pkMsg is of non-class type SQLMsg* means you are using .reset on a raw pointer (and .reset is not needed even for smart ptr).

You solved differently but maybe someone is interested in knowing why errors are received when they are received, as well as knowing how to fix it.

 

 

 

  

On 11/7/2020 at 2:21 PM, developerares said:

Is there a problem with this code? I have smart ptr. 

 

No it is correct. std::unique_ptr<SQLMsg> will delete the memory from heap when its destructor is called. 

smart pointers have the overload of the operator -> and this help to use them as classic (raw) pointers. (e.g. where you used Msg->Get())

Edited November 8, 2020 by Ikarus_

[developerares 1]

Thank you @V0lvox and @Ikarus_

[Flourine 102]

Just use smart pointers