Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add validations/alerts for packet ranges #3309

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

guilherme-gm
Copy link
Member

Pull Request Prelude

Changes Proposed

  • Add assert to ensure MAX_PACKET_DB is within 2 bytes range (currently packetType is 2 bytes long)
  • Add error message when HPM plugins declare packets outside of PACKET_DB range. This is currently silently ignored. Making a error message will help users debugging their plugins.

Issues addressed:
None, discussed in discord; thanks to csnv and dastgirp for bringing these up

this makes addPacket macro for HPM plugins to properly alert users when their
packets are ignored due to being out of the supported range,
this was previously silently ignored
@guilherme-gm
Copy link
Member Author

Uhh... I am really confused now.

CI is failing because httpsample registers packets below MIN_PACKET_DB:

addProxyPacket(API_MSG_SAMPLE_LOGIN /* 20 < 36 */, sample_login_request, sample_login_api_packet, hpProxy_ApiLogin);

But looking at the list of packets from the API-server, all of them are < 36, even the core ones. For those added by HPM, it would mean their length are not added to packets->db[packet_id]. But I guess... they work?

Checking how HPM handles it, it seems to have a loop before the main packet handling that if it finds this packet, it does everything itself (even for non API ones):

		if (VECTOR_LENGTH(HPM->packets[hpParse_FromLogin]) > 0) {
			int result = HPM->parse_packets(fd,command,hpParse_FromLogin);
			if (result == 1) // Parsed and handled
				continue;
			if (result == 2) // Incomplete packet
				return 0;
		}

and HPM->parse_packets makes use of another field (packet->len). So I am not exactly sure when/if packets->db[packet_id] is needed for packets loaded by HPM...

Anyways, now I am in doubt about the better approach here... Adding something out of the supported range does seem wrong, so it would be better to just change the sample. But I am wondering why the API-server is fine with that and whether we really want to keep it outside the range.

@4144 / @MishimaHaruna / @skyleo any suggestions?

@guilherme-gm guilherme-gm marked this pull request as draft August 10, 2024 23:47
@MishimaHaruna MishimaHaruna added this to the Release v2024.09 milestone Aug 31, 2024
@MishimaHaruna
Copy link
Member

I'm looking at how those packets work and if I'm understanding it right I think it's a bit of a hack in order to reuse the HPM's packet id/handler dispatch logic for the proxied API messages.

Even though they're inserted into the HPM's packet database (in dedicated sections, hpProxy_ApiLogin, hpProxy_ApiChar, hpProxy_ApiMap), those aren't actually used as packet IDs, but rather the msg_id field of struct PACKET_API_PROXY. The choice of using IDs outside the valid packet ranges might have been deliberate, so that hplugins_addpacket() will register them into HPM->packets[point][id] but not into packet->db[id]. (@4144 might be able to shed some light on this)

You can see how they're used i.e. in mapiif_parse_fromchar_api_proxy (but there is similar code in capiif_parse_fromlogin_api_proxy and lapiif_parse_fromapi_api_proxy), where, after unwrapping the actual proxy packet, it calls HPM->parse_packets() with the msg_id instead of the packet_id:

static int mapiif_parse_fromchar_api_proxy(int fd)
{
	RFIFO_API_PROXY_PACKET(packet);
	const uint32 msg = packet->msg_id;

	if (VECTOR_LENGTH(HPM->packets[hpProxy_ApiMap]) > 0) {
		int result = HPM->parse_packets(fd, msg, hpProxy_ApiMap);
		if (result == 1)
			return 1;
		if (result == 2) {
			RFIFOSKIP(fd, packet->packet_len);
			return 1;
		}
	}

	switch (msg) {
	case API_MSG_party_info:
		mapiif->parse_adventurer_agency_info(fd);
		break;
	default:
		ShowError("Unknown proxy packet 0x%04x received from char-server, disconnecting.\n", msg);
		sockt->eof(fd);
		return 0;
	}

	RFIFOSKIP(fd, packet->packet_len);
	return 1;
}

For the purpose of this PR, I think you can simply skip the error reporting if point is one of hpProxy_ApiLogin, hpProxy_ApiChar, hpProxy_ApiMap since those are the ones dedicated to this msg_id mechanism.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants