Refactor ClientCard memory management

[salix5]

Problem

case MSG_MOVE: {
		unsigned int code = BufferIO::Read<int32_t>(pbuf);
		int pc = mainGame->LocalPlayer(BufferIO::Read<uint8_t>(pbuf));
		unsigned int pl = BufferIO::Read<uint8_t>(pbuf);
		int ps = BufferIO::Read<uint8_t>(pbuf);
		unsigned int pp = BufferIO::Read<uint8_t>(pbuf);
		int cc = mainGame->LocalPlayer(BufferIO::Read<uint8_t>(pbuf));
		unsigned int cl = BufferIO::Read<uint8_t>(pbuf);
		int cs = BufferIO::Read<uint8_t>(pbuf);
		unsigned int cp = BufferIO::Read<uint8_t>(pbuf);
		int reason = BufferIO::Read<int32_t>(pbuf);
		if(!mainGame->dInfo.isReplay || !mainGame->dInfo.isReplaySkiping) {
			if(cl & LOCATION_REMOVED && pl != cl)
				soundManager.PlaySoundEffect(SOUND_BANISHED);
			else if(reason & REASON_DESTROY && pl != cl)
				soundManager.PlaySoundEffect(SOUND_DESTROYED);
		}
		int appear = mainGame->gameConf.quick_animation ? 12 : 20;
		if (pl == 0) {
			ClientCard* pcard = new ClientCard;
			pcard->position = cp;
			pcard->SetCode(code);
			if(!mainGame->dInfo.isReplay || !mainGame->dInfo.isReplaySkiping) {
				mainGame->gMutex.lock();
				mainGame->dField.AddCard(pcard, cc, cl, cs);
				mainGame->gMutex.unlock();
				mainGame->dField.GetCardLocation(pcard, &pcard->curPos, &pcard->curRot, true);
				pcard->curAlpha = 5;
				mainGame->dField.FadeCard(pcard, 255, appear);
				mainGame->WaitFrameSignal(appear);
			} else
				mainGame->dField.AddCard(pcard, cc, cl, cs);
		}

void ClientField::AddCard(ClientCard* pcard, int controler, int location, int sequence) {
	pcard->controler = controler;
	pcard->location = location;
	pcard->sequence = sequence;
	switch(location) {
	case LOCATION_DECK: {
		if (sequence != 0 || deck[controler].size() == 0) {
			deck[controler].push_back(pcard);
		} else {
			deck[controler].insert(deck[controler].begin(), pcard);
		}
		ResetSequence(deck[controler], true);
		pcard->is_reversed = false;
		pcard->ClearData();
		pcard->ClearTarget();
		SetShowMark(pcard, false);
		break;
	}
	case LOCATION_HAND: {
		hand[controler].push_back(pcard);
		ResetSequence(hand[controler], false);
		break;
	}
	case LOCATION_MZONE: {
		mzone[controler][sequence] = pcard;
		break;
	}
	case LOCATION_SZONE: {
		szone[controler][sequence] = pcard;
		break;
	}
	case LOCATION_GRAVE: {
		grave[controler].push_back(pcard);
		pcard->sequence = (unsigned char)(grave[controler].size() - 1);
		break;
	}
	case LOCATION_REMOVED: {
		remove[controler].push_back(pcard);
		pcard->sequence = (unsigned char)(remove[controler].size() - 1);
		break;
	}
	case LOCATION_EXTRA: {
		if(extra_p_count[controler] == 0 || (pcard->position & POS_FACEUP)) {
			extra[controler].push_back(pcard);
		} else {
			size_t faceup_begin = extra[controler].size() - extra_p_count[controler];
			extra[controler].insert(extra[controler].begin() + faceup_begin, pcard);
		}
		ResetSequence(extra[controler], true);
		if (pcard->position & POS_FACEUP)
			extra_p_count[controler]++;
		break;
	}
	}
}

MSG_MOVE, pl = 0
If cl is wrong, dField.AddCard will fail.
The pointer pcard will not be placed to any container, and it is lost after leaving the block.
It will causes memory leak.

Reason

All ClientCard object should be managed by ClientField, instead of creating new ClientCard object at any place.

Solution

ClientField

std::vector<std::unique_ptr<ClientCard>> cards_;

Now it will keep track of ClientCard unique_ptr in the private member cards_.

ClientCard

Now it has a pointer to the ClientField object.

@Wind2009-Louse

[Copilot]

Pull request overview

This PR refactors ClientCard lifetime management to be owned by ClientField (via std::unique_ptr storage), replacing scattered new/delete sites and reducing the risk of leaking cards when they fail to be inserted into a field container.

Changes:

• Introduces ClientField::CreateCard() / ClientField::DestroyCard() and a private cards_ owner container.
• Updates DuelClient to allocate/free cards through ClientField instead of new/delete.
• Makes ClientCard field-aware (stores a ClientField*) and routes certain operations through it.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
gframe/duelclient.cpp	Switches card allocation/deallocation to CreateCard()/DestroyCard() in multiple message handlers.
gframe/client_field.h	Adds CreateCard()/DestroyCard(), includes <memory>, and introduces the owning cards_ container.
gframe/client_field.cpp	Implements CreateCard()/DestroyCard() and removes manual deletion loops in Clear()/destructor.
gframe/client_card.h	Replaces default ctor with ClientCard(ClientField*) and stores a ClientField*.
gframe/client_card.cpp	Uses field_ for field interactions and creates overlay cards via field_->CreateCard().

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[mercury233]

Even after this change, there is still some potential memory-safety issues. In MSG_TAG_SWAP, after DestroyCard is called, containers such as selectable_cards and display_cards may still hold references to the destroyed cards.

For example, in a tag duel replay, if the user opens the opponent's Deck view before the first player's turn ends, the end-of-turn tag swap will update the Deck contents. The already-open card list can then display incorrect cards, or potentially reference cards that have been destroyed.

Actually, our support for viewing zones such as the Graveyard during replay is already fragile. We currently avoid stale selectable_cards issues mostly by forcibly closing wCardSelect before ClientAnalyze processes new messages.

[salix5]

Better than nothing, I suppose.

[mercury233]

My understanding is that the goal of this PR is to reduce the risk of memory leaks, but it doesn't address the existing dangling pointer/use-after-free risks, and may actually increase them. If that's the case, I don't think it should be merged now.

[salix5]

My understanding is that the goal of this PR is to reduce the risk of memory leaks, but it doesn't address the existing dangling pointer/use-after-free risks, and may actually increase them. If that's the case, I don't think it should be merged now.

Can you give any example on how this PR "increase" dangling pointer?
It just replaces delete with a new function.

[salix5]

Memory leak is a big problem by itself.

If you are trying to say:
This PR only solves 1 big problem, instead of solving 2 big problems at the same time, so it is not good enough.

I am afraid that this is not very convincing.

[mercury233]

Memory leaks are probably the least serious issue among the "big problems", especially on the client side. A ClientCard is less than 1 KB, and over the course of a 20-minute game, it's unlikely that more than a handful of them would ever be leaked.

I'm not saying memory leaks shouldn't be fixed, nor am I saying it's impossible to fix both memory leaks and UAFs at the same time. My point is that UAFs are much more dangerous, and in many cases, strategies for eliminating memory leaks can be at odds with strategies for preventing UAFs.

For this particular PR, I can't point to a specific place where it increases the risk of UAF. Just as static analysis and unit tests can't catch every bug, I can't prove that it does. My concern is that I don't think it's the right direction, or at least not the direction I would choose to pursue.

[salix5]

My understanding is that the goal of this PR is to reduce the risk of memory leaks, but it doesn't address the existing dangling pointer/use-after-free risks, and may actually increase them. If that's the case, I don't think it should be merged now.

First of all:
This PR does not increase UAF or dangling pointer.
It just replaces delete with a new function.
If ygopro did use a pointer after freed, it already did that before this PR.
I suppose that it is not the case, and your guess might be wrong.

About UAF

The root cause of UAF is creating and destroying objects at any place without being noticed by other functions.

The main purpose of this PR:
The construction and destruction of ClientCard objects are managed by the vector of smart pointers in ClientField.

By using an array of smart pointers to keep of all active cards, this PR is actually the first step of fixing UAF.
At least, it is possible to check if a pointer is valid by checking cards vector.

[salix5]

My point is that UAFs are much more dangerous, and in many cases, strategies for eliminating memory leaks can be at odds with strategies for preventing UAFs.

Again, can you give any example on "strategies for eliminating memory leaks can be at odds with strategies for preventing UAFs"?

[mercury233]

A quick skim of the code wasn't enough for me to fully understand how this PR works. For the reasons I mentioned earlier, I'm not planning to dig into it in depth for now.

Centralizing ownership/management may be the first step toward fixing UAFs, or it may be the first step toward introducing even more bugs (though I also feel the former is more likely).

can you give any example

One common approach to fixing memory leaks is to add more free/delete calls, whereas UAFs are often caused by calling free or delete at the wrong time or in the wrong place.

I'm no longer voting against it. I'll just not vote.

[salix5]

One common approach to fixing memory leaks is to add more free/delete calls, whereas UAFs are often caused by calling free or delete at the wrong time or in the wrong place.

This PR fix memory leaks by using a vector of smart pointers managed by ClientField.
The vector will be cleared or destroyed when Client Field is cleared or destroyed.

If you really worry about use-after-free, then don't free any ClientCard object as long as ClientField is alive.
The cards will only be removed from deck or mzone, but the memory is not freed.
As you said, ClientCard objects are small, and keeping all of them would not be a problem.

I suggest that remove all delete from the code, and the memory will be freed only when ClientField is cleared or destroyed.