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

fix(app): fixed a bug in geoip matching with refactoring. #2489

Merged
merged 1 commit into from
Aug 26, 2023

Conversation

cty123
Copy link
Contributor

@cty123 cty123 commented Aug 24, 2023

As raised in #1933, the existing in-house CIDR matching mechanism can produce incorrect result in some cases. The issue was originally brought up from v2ray-core project v2fly/v2ray-core#1155. This PR ported most of the commit from the v2ray-core commit v2fly/v2ray-core#1157 by @Loyalsoldier.

Here's an example for the bug(also as a new test cases I added with the PR),

func TestGeoIPMatcherRegression(t *testing.T) {
	cidrList := []*router.CIDR{
		{Ip: []byte{98, 108, 20, 0}, Prefix: 22},
		{Ip: []byte{98, 108, 20, 0}, Prefix: 23},
	}

	matcher := &router.GeoIPMatcher{}
	common.Must(matcher.Init(cidrList))

	testCases := []struct {
		Input  string
		Output bool
	}{
		{
			Input:  "98.108.22.11",
			Output: true,
		},
		{
			Input:  "98.108.25.0",
			Output: false,
		},
	}

	for _, testCase := range testCases {
		ip := net.ParseAddress(testCase.Input).IP()
		actual := matcher.Match(ip)
		if actual != testCase.Output {
			t.Error("expect input", testCase.Input, "to be", testCase.Output, ", but actually", actual)
		}
	}
}

If you copy this over to the condition_geoip_test.go file on main branch, the test would fail.

--- FAIL: TestGeoIPMatcherRegression (0.00s)
    /home/ctydw/Projects/Xray-core/app/router/condition_geoip_test.go:154: expect input 98.108.22.11 to be true , but actually false

But this is not really expected, because 98.108.20.0/22 is from 98.108.20.0 to 98.108.23.255, so it is in the range. Instead of fixing the algorithm itself, I do think it makes sense to replace the in-house algorithm with the netipx library.

@RPRX
Copy link
Member

RPRX commented Aug 26, 2023

感谢 PR,看起来这两个 test 需修复:TestGeoIPMatcher6US、TestRoutingRule

另外请基于 main 分支 rebase,并升级(若有)新引入的依赖 go4.org/netipx

@RPRX
Copy link
Member

RPRX commented Aug 26, 2023

担心的事情还是发生了,麻烦重新 rebase 一下。。。

@RPRX
Copy link
Member

RPRX commented Aug 26, 2023

看起来 test 已经没问题了

@cty123
Copy link
Contributor Author

cty123 commented Aug 26, 2023

没事,反正我能force push,之前写错一个小地方,debug了半天发现是低级错误。。。

* Refactor the IP address matching with netipx library
* Add a regression test for previous bug
@RPRX RPRX merged commit b24a402 into XTLS:main Aug 26, 2023
35 checks passed
@RPRX
Copy link
Member

RPRX commented Aug 26, 2023

感谢 PR

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.

2 participants