-
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
Bug when comparing size of len(msg.Pack) and msg.Len() #663
Comments
Here is a failing Test Case: func TestMsgCompressLengthLargeRecordsAllValues(t *testing.T) {
msg := new(Msg)
msg.Compress = true
msg.SetQuestion("redis.service.consul.", TypeSRV)
for i := 0; i < 900; i++ {
target := fmt.Sprintf("host-redis-%d-%d.test.acme.com.node.dc1.consul.", i/256, i%256)
msg.Answer = append(msg.Answer, &SRV{Hdr: RR_Header{Name: "redis.service.consul.", Class: 1, Rrtype: TypeSRV, Ttl: 0x3c}, Port: 0x4c57, Target: target})
msg.Extra = append(msg.Extra, &CNAME{Hdr: RR_Header{Name: target, Class: 1, Rrtype: TypeCNAME, Ttl: 0x3c}, Target: fmt.Sprintf("fx.168.%d.%d.", i/256, i%256)})
predicted := msg.Len()
buf, err := msg.Pack()
if err != nil {
t.Error(err)
}
if predicted != len(buf) {
t.Fatalf("predicted compressed length is wrong for %d records: predicted %s (len=%d) %d, actual %d", i, msg.Question[0].Name, len(msg.Answer), predicted, len(buf))
}
}
} It fails with the following:
|
[ Quoting <notifications@github.com> in "[miekg/dns] Bug when comparing size..." ]
Following #635 I updated DNS lib to 1.0.5 into Consul but ran into another issue and I am still able to reproduce case where len(msg.Pack()) is not equal to msg.Len()
There could, maybe, be an one-off in Len(), but I don't have the time to deepdive
that atm.
|
@miekg I tried to find the issue... it seems it is related to the compression of data of Extra... I was not exactly able to find what is the issue, but this is definitly related to tests such as I'll try to find some time to dig a bit more |
[ Quoting <notifications@github.com> in "Re: [miekg/dns] Bug when comparing ..." ]
@miekg I tried to find the issue... it seems it is related to the compression of data of Extra... I was not exactly able to find what is the issue, but this is definitly related to tests such as `len < maxCompressionOffset`...
I'll try to find some time to dig a bit more
Thanks.
Yes, the `len < maxCompressionOffset` should maybe be `<=`, but I haven't given
this a lot of thought.
|
It took me a while to figure out the issue (a few hours), it was too simple :-) Since len was added before trying to add label to compression and since it is legal to have a start offset before 14 bits, we just had to more the increase of len at the end of loop :-) |
@miekg Ok, the patch I made seems to work fine now. I added lots of tests cases, including TestMsgCompressLengthLargeRecordsWithPaddingPermutation which test all permutations with padding around 14bits, so it should work well in all cases now |
Following #657 I updated DNS lib to 1.0.5 into Consul but ran into another issue and I am still able to reproduce case where len(msg.Pack()) is not equal to msg.Len()
pierresouchay/consul@f85db7e
It crashes with the following output:
I am gonna try to provide a test case
The text was updated successfully, but these errors were encountered: