Exchange() transparently uncompresses message without compress flag set

[discordianfish]

Hi,

if you use Exchange() to get compressed DNS response which still fits in 512 byte, you end up with a Msg larger than 512 and without compress flag set. That means if you write this message somewhere, it won't get compressed and you end up with a invalid message (udp, no EDNS, >512 bytes). Haven't test it but I think https://github.com/miekg/dnsrouter/blob/master/server.go#L119 should be affected as well.

This probably only affects stub resolvers not supporting EDNS, most notably netgo.

[miekg]

As you said on twitter, probably always setting Compress = true is best.

[discordianfish]

Can we check if the received message was compressed and only set compressed flag in this case?

[miekg]

I wonder if I should make this the default in msg.Unpack or putting it in Exchange only...

[miekg]

Yes, you can see if you followed compression pointers... But this info is buried deep inside the function and not exported. Setting it in Unpack might mess with dynamic update packets. I'll set it in exchange and friends

[miekg]

See compression branch which should fix this.

[discordianfish]

@miekg Wouldn't it be cleaner to only set it if the message was compressed? People might not want to compress the messages. At least @armon had some concerns about always compressing: hashicorp/consul#971 (comment)

[miekg]

[ Quoting [email protected] in "Re: [dns] Exchange() transparently ..." ]

@miekg Wouldn't it be cleaner to only set it if the message was compressed? People might not want to compress the messages. At least @armon had some concerns about always compressing: hashicorp/consul#971 (comment)

yes, but a lot more work and ugly refactoring.

Question: why are these messages send to? Why not just set compression there
(i.e. in the consul code?)

[discordianfish]

@miekg See the issue I linked to, @armon had some concerns about corrupted messages.

[miekg]

but concerns, or actual corruption?
On 5 Jun 2015 4:38 pm, "discordianfish" [email protected] wrote:

@miekg https://github.com/miekg See the issue I linked to, @armon
https://github.com/armon had some concerns about corrupted messages.

—
Reply to this email directly or view it on GitHub
#216 (comment).

[miekg]

Those comments say not much at all. Coming back to my question, why can't you set compression yourself? I do the same in SkyDNS.

[discordianfish]

@miekg Sure it's possible but I'm not a consul maintainer. Guess it would make sense to fix your dnsrouter and document that Exchange might return a message which is too big to be sent to a client unless compressed, then do the ugly refactoring sometime in the future.

[miekg]

Don't think this is a transitive property of the dns message. I don't store (in the message) if it came via UDP or TCP. This should be set on a per-connection basis.

[jameshartig]

We recently ran into this issue where it was originally compressed before unpacking and then when re-packed it was not compressed. If the original message was compressed, I think it makes sense for msg.Compress to be true. Just like how if a message was truncated, msg.Truncated is true.

Though... we are also just setting compress on every single message now after running into this problem.

[miekg]

[ Quoting [email protected] in "Re: [dns] Exchange() transparently ..." ]

message was compressed, I think it makes sense for msg.Compress to be true.
Just like how if a message was truncated, msg.Truncated is true.

But msg.Truncated and msg.Compress are 2 entirely different things. Truncated
is actually set by the server. Changing that bit, changes the semantics of the
message. Compress OTOH is just meta data on this message in this communication
between this client and server. The fact that a message was send via TCP is
also not stored in the Msg struct...

/Miek

Miek Gieben

[andrewtj]

Setting Msg.Compress if a compression pointer is encountered during unpacking won't guarantee that a message will repack to the same size. Don't rely on Msg.Compress in place of a payload size check.

[andrewtj]

Closing as there hasn't been any activity for a while and it doesn't look like there is anything outstanding.

[hermansc]

I don't think this should be closed, as the original issue has not been fixed.

[andrewtj]

@hermansc If you've read through this issue and have something to add please do so.

[hermansc]

Sorry, I just came here from hashicorp/consul#854 (comment) and since we're still having issues in Consul and this wasn't fixed I thought I'd let you know.

[andrewtj]

@hermansc No worries. For my 2¢ (and not necessarily @miekg's), there's nothing to do here, or at least nothing that doesn't involve deprecating Msg.Compress. In time I'd expect to see the signal to compress move elsewhere (and not be a boolean), but for the present, it might be a bit odd at first but it's not difficult to work with.