-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
When Compress is enable AND size if large, msg.Len() != len(msg.Pack()) #657
Comments
In trying to minimise this test case I ran in to the following:
|
@andrewtj yes, seems linked to SRV records as my patch did not had issues with A/AAAA records |
@pierresouchay the same behaviour occurs (with my example) using a CNAME instead of an SRV. I'm not sure we're exercising the same bug, but I'm not up to speed with the current compression and len implementation so I'm just hazarding a guess. If you have a bit of time to put into this but don't want to dig in to the implementation itself (understandable), it might be interesting to see if your example shows up after a particularly offset as only names appearing at an offset that fits in 14-bits can be used as compression targets (RFC3597 might come in useful for padding). Aside: SRV shouldn't be compressed anyway, but that's #521. |
oh yeah, the 14 bit limit that's not implemented in Len() (forgot about that one). |
Note the CNAME header type in the test above is set to 0x1 which is Clearing up the test to the minimum shows it happens (in case) with 250 records, so we keep missing 17 octets, which Pack seems to save, but Len() doesn't. Some hackish println-ing shows that all headers that can be compressed are compressed. Now wondering where 17 comes from and why...
|
|
Pushed code that has my current code: #658 |
@miekg yes, i did copy the code of another test, just to reproduce the issue on Consul's codebase, this was just to prove my claims |
Ah, |
Fixes Len() to take maxCompressionOffset into account. It doesn't fail on the test in length_test.go and to make sure I also used to above test code as well (which also works). |
closed via #658 |
I found another issue #663 (comment) |
While working on a patch for Consul hashicorp/consul#3948 I found a bug in this library because when compression is activated, msg.Len() is not the same as len(msg.Pack())
Since Consul was using a very old version of the library, I thought it had been fixed, so Consul upgraded its version to 1.0.4 after this issue hashicorp/consul#3977
But still, the issue still exists with version 1.0.4.
I created a test case that shows it into action: (to paste into
dns_test.go
):This test fails this way:
Which is exactly what I did see while patching Consul.
In the patch I am working on, since Consul might create very large SRV records (several thousands), in order to have a good response, we are trying to fill DNS at its maximum, so we compute the size compressed, see if it is more than 64K, if not send the response, otherwise, remove records and retry.
So, basically, the size uncompressed is supposed to be far bigger than 64k while I am trying to improve the patch to send 64k MAX compressed.
Note that this test passes without any issue with compression disabled.
The text was updated successfully, but these errors were encountered: