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

Remove usages of Game.convertIpToString and mark as deprecated #4283

Merged
merged 2 commits into from
Apr 5, 2023

Conversation

ranisalt
Copy link
Member

Pull Request Prelude

Changes Proposed

Fixes a backwards-incompatibility introduced in #3952 where there are still some functions around that expect numeric IPs, which are not available anymore. Since most of the time the IP comes from player:getIP(), which returns string values, this function can be skipped, but if they come from somewhere else it will still try to convert the number.

Issues addressed:
#4274

@EPuncker EPuncker linked an issue Dec 27, 2022 that may be closed by this pull request
EPuncker
EPuncker previously approved these changes Dec 27, 2022
local result = {}
for _, player in ipairs(Game.getPlayers()) do
if bit.band(player:getIp(), mask) == masked then
if player:getIp() == masked then
Copy link
Contributor

@Zbizu Zbizu Dec 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you have a nil value there, variable masked no longer exists within this function scope after your changes

Copy link
Contributor

@MillhioreBT MillhioreBT Jan 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should look something like this right?

function getPlayersByIPAddress(ip, mask)
	local players = {}
	if type(ip) == "string" then
		for _, player in ipairs(Game.getPlayers()) do
			if player:getIp() == ip then
				players[#players + 1] = player:getId()
			end
		end
	else
		if not mask then mask = 0xFFFFFFFF end
		local masked = bit.band(ip, mask)
		for _, player in ipairs(Game.getPlayers()) do
			if bit.band(player:getIp(), mask) == masked then
				players[#players + 1] = player:getId()
			end
		end
	end
	return players
end

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MillhioreBT Would be nice to have some function to convert string IP to numeric so mask would also be applied for strings. But anyway as there's IPv6 support (#3952) it should handle prefixes too ("subnet mask").

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But actually as this is a only for compatibility it should be okay to accept numeric input only as all usecases in user scripts are numeric*.
One problem is comparison player:getIp() (string) with ip (number) and another is IPv6 prefix masks.

(*) But also return types of getIPByPlayerName/getIpByName were changed from number to string so it's no longer valid that getPlayersByIPAddress would always get numeric IP's.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xmish yes, that's the reasoning. I just kept this compatibility of accepting numeric IPs for the extremely unlikely, but definitely possible scenario where someone manually blocked an IP by hardcoding it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Zbizu hope it is good enough now 😅

@omarcopires
Copy link
Contributor

The advantage is that the error no longer happens, however the ip appears as follows:
image

@ranisalt
Copy link
Member Author

ranisalt commented Feb 5, 2023

@omarcopires does this print just the return value of player:getIp(), or are you still calling Game.convertIpToString()?

@omarcopires
Copy link
Contributor

@omarcopires does this print just the return value of player:getIp(), or are you still calling Game.convertIpToString()?

I'm using the code as in your branch, I didn't make any changes.

@xmish
Copy link
Contributor

xmish commented Feb 5, 2023

The advantage is that the error no longer happens, however the ip appears as follows: image

However it's valid representation of IPv4, see: RFC4291.
It's what you get of boost::asio::ip::address::to_string, Solution I've found:

    boost::asio::ip::address addr{boost::asio::ip::make_address("::ffff:127.0.0.1")};
    boost::asio::ip::address_v6 ipv6 = addr.to_v6(); // convert to address_v6 
    if (ipv6.is_v4_mapped()) {
        auto ipv4 = ipv6.to_v4();
        std::string str = ipv4.to_string(); // 127.0.0.1
    }

@ranisalt
Copy link
Member Author

ranisalt commented Feb 5, 2023

I'm using the code as in your branch, I didn't make any changes.

As @xmish pointed out, that's a valid IP, and your server console is printing that the function has been deprecated, where it has been called (filename and line), and that you should skip calling it instead. What do you think, is this enough information for the server owner?

@omarcopires
Copy link
Contributor

I'm using the code as in your branch, I didn't make any changes.

As @xmish pointed out, that's a valid IP, and your server console is printing that the function has been deprecated, where it has been called (filename and line), and that you should skip calling it instead. What do you think, is this enough information for the server owner?

I didn't realize that was the default behavior, sorry.
This is more than enough.

@EPuncker EPuncker merged commit bcf7eda into otland:master Apr 5, 2023
@ranisalt ranisalt deleted the fix-convert-ip branch April 5, 2023 11:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

look me
6 participants