Handle packing of empty RDATA correctly for EDNS0_EXPIRE (Resolves #1292)

[dmavrommatis]

Issue explained in #1292

[miekg]

Thanks. Could you provide a better description in your commit. uint32 was plain wrong? Where in the RFC do I need to look? Etc.

[dmavrommatis]

I've documented the bug and thought process in the issue mentioned. I've also changed the description in the commit to also mention the RFC and the issue:

As per RFC7314 the Expire Option in queries should be zero-length. In the current implementation the field is uint32 which always instatiates 4bytes for that field when packing to wire format.
For that reason we change the field to []uint8 so it can support 0-length and 4-byte
length option data.

[dmavrommatis]

@miekg thanks for the review; I've addressed the comments. Please take a look again!

[tmthrgd]

(Apologies for the absence on GitHub).

RFC 7314 says "This is done by adding a zero-length EDNS EXPIRE option to the options field of the OPT record when the query is made." so a zero length option is valid and something we should be handling. There's nothing in the RFC to say that the 32-bit value zero is reserved so we have to persist this somehow.

This PR isn't backward compatible though so we shouldn't merge as-is. What we could do is hang a flag off EDNS0_EXPIRE and use that while keeping everything else the same. Perhaps Empty or ZeroLength or whatever.

[dmavrommatis]

(Apologies for the absence on GitHub).

RFC 7314 says "This is done by adding a zero-length EDNS EXPIRE option to the options field of the OPT record when the query is made." so a zero length option is valid and something we should be handling. There's nothing in the RFC to say that the 32-bit value zero is reserved so we have to persist this somehow.

This PR isn't backward compatible though so we shouldn't merge as-is. What we could do is hang a flag off EDNS0_EXPIRE and use that while keeping everything else the same. Perhaps Empty or ZeroLength or whatever.

Can you elaborate for the backwards compatibility issue?
These functions are for packing and unpacking a message to bytes and the other way around. How will introducing a flag going to provide anything? In the end we will either write 0 bytes or 4 bytes.

[tmthrgd]

Any code that operates on EDNS0_EXPIRE would be broken by this as the type of Expire will have changed. Consider code that did something like (nonsense example):

func adjustExpire(opt *EDNS0_EXPIRE) {
	if opt.Expire < 10 {
		opt.Expire = 10
	} else {
		opt.Expire++
	}
}

Even code that simply inspects or prints the value of Expire will break.

A flag allows us to track whether we're writing zero or four bytes and persist that from unpack to pack without (hopefully) breaking any existing code.

[dmavrommatis]

Any code that operates on EDNS0_EXPIRE would be broken by this as the type of Expire will have changed. Consider code that did something like (nonsense example):

func adjustExpire(opt *EDNS0_EXPIRE) {
	if opt.Expire < 10 {
		opt.Expire = 10
	} else {
		opt.Expire++
	}
}

Even code that simply inspects or prints the value of Expire will break.

A flag allows us to track whether we're writing zero or four bytes and persist that from unpack to pack without (hopefully) breaking any existing code.

oh gotcha, I will update the PR to avoid this issue

[dmavrommatis]

@tmthrgd found some time to make the change. please check to see if the implementation is sufficient.

[dmavrommatis]

hey @tmthrgd I think this PR is ready for merging!

[dmavrommatis]

@tmthrgd @miekg can you check if the PR is complete now?

[tmthrgd]

LGTM. As long as we're happy with the String output.