jsommers / pytricia

A library for fast IP address lookup in Python.
GNU Lesser General Public License v3.0
214 stars 22 forks source link

Wrong response when an IPv6 prefix is not found #29

Closed bortzmeyer closed 4 years ago

bortzmeyer commented 4 years ago
>>> import pytricia
>>> pyt = pytricia.PyTricia(128)
>>> pyt.insert('32.0.0.0/8', "a")
>>> pyt.get_key('2009:b98:dc0:41:216:3eff:fe27:3d3f')
'32.0.0.0/8'
>>> pyt.get_key('1.2.3.4')
>>> 

For IPv4, this is the expected result (None) but for IPv6, it returns something???

This may be related to bug #19.

bortzmeyer commented 4 years ago

I tried to specify explicitely the prefix length but it changed nothing.

bortzmeyer commented 4 years ago

The workaround I used was to have two Patricia tries, one for IPv4 and one for IPv6.

ikapelyukhin commented 4 years ago

I was trying to figure out whether it is a good idea to put IPv4 and IPv6 addresses into the same tree and came upon this issue.

This works the other way around as well:

>>> import pytricia
>>> pyt = pytricia.PyTricia(128)
>>> pyt.insert('2000::/8', 'test')
>>> pyt.get_key('32.0.0.0')
'2000::/8'

I guess this is due to the fact that the first octet of 32.0.0.0 is 00100000 in binary; and the first two octets of 2009:b98:dc0:41:216:3eff:fe27:3d3f are 0010000000001001 -- and with /8 prefix only the first 8 bits have to match (and they match).

So I guess it is a bad idea to put IPv4 and IPv6 into the same tree. Perhaps the documentation should specify this explicitly?

bortzmeyer commented 4 years ago

@ikapelyukhin I agree (with experience...) that it is a bad idea to put both families in the same trie, and I agree that documenting it could be a good idea.

bortzmeyer commented 4 years ago

By the way, the code which uses it is https://framagit.org/bortzmeyer/whichasn (see daemon.py)

ikapelyukhin commented 4 years ago

I suppose IPv4 addresses can be mapped to IPv6, but that requires converting the addresses back and forth.

bortzmeyer commented 4 years ago

I suppose IPv4 addresses can be mapped to IPv6, but that requires converting the addresses back and forth.

Clever idea :-)

jsommers commented 4 years ago

Agree that the documentation should address this explicitly, that mixing v4 and v6 addresses in the same trie is generally not a good idea. I think this is purely a documentation update issue.

0xxon commented 4 years ago

To provide some additional feedback - would it perhaps be possible to raise an exception when checking an IPv6 address against a structure containing IPV4 addresses and vice versa? So - to type them?

I would definitely have run into this one if I had not randomly seen this issue.