Invalid NSEC/3 bitmap on non-zero buffer #1338
Conversation
If the PackBuffer is used to encode an NSEC/3 record, the bitmap is xored with the content of the buffer instead of being zeroed first. The algorithm has been changed so it is able zero bytes without losing too much performance (around 2x slower).
|
What’s the actual issue here? Is it observable using the packages public API? |
|
I creates invalid DNS messages when This is randomly breaking DNSSEC when using |
|
[ Quoting ***@***.***> in "Re: [miekg/dns] Invalid NSEC/3 bitm..." ]
I creates invalid DNS messages when PackBuffer is used with a non-zeroed
buffer. The existing tests would actually not pass if the generated content was
tested (like it is now added).
This is randomly breaking DNSSEC when using PackBuffer.
Think this should added to the function docs and you should clear the buffer, no need to
impose that cost on every user.
|
|
When you reuse buffers, you are not expected to clear them, that would defeat the purpose of reusing buffers. Your own existing tests was expecting a non zero buffer and was failing to generate a valid message. The performance difference is 10ns, it’s not a big cost. Zeroing the full buffer would cost more. |
|
ok, I don't like the entire re-write of the loop, I could understnad the first iteration. Please see if there is a more minimal change, also use the go std naming scheme, so no underscores like |
|
First loop is for window blocks of high order 8 bits and the second is to encode the low order 8 bits bitmaps and the third is to find all additional types fitting in the same bitmap. There is another loop to make sure we zero empty bitmaps before the current one if any. I don't see a more efficient way to do it with less loops. Previous implementation couldn't know which bytes to 0. I'll rename |
|
Please have a look at the last commit. I renamed a few vars + added some comments for clarity. |
|
@miekg are you considering merging this fix? |
|
Considering we only advance |
|
You don't know the number of lower order 8bit bitmap blocks in advance, and the previous algorithm had no way to know if the current block its writing in needs to be zeroed. The previous algorithm also not give you a hint on the number of blocks was skipped and thus need to be zeroed as well. |
|
But we can trivially know the maximum |
|
Sure you can, it will be another loop and performance will be probably the same or worth. |
|
I reverted to the previous algo with window size compute+0 on each window boundaries. |
|
@miekg are you still considering merging this fix? |
|
It depends on the size of |
|
With a 500 bytes |
|
Even when using the assembly backed |
|
This is with this construct yes. |
|
I just noticed we already have toZero := msg[off:]
if maxLen := typeBitMapLen(bitmap); maxLen < len(toZero) {
toZero = toZero[:maxLen]
}
for i := toZero {
toZero[i] = 0
} |
|
Awersome, I moved to this function. Please review. |
| @@ -558,6 +558,16 @@ func packDataNsec(bitmap []uint16, msg []byte, off int) (int, error) { | |||
| if len(bitmap) == 0 { | |||
| return off, nil | |||
| } | |||
| if off > len(msg) { | |||
There was a problem hiding this comment.
I don't think it's possible for us to ever be called with off > len(msg), so I think this check is unneeded.
There was a problem hiding this comment.
Famous last word :)
The first unit test was testing this and would panic without this test. The cost is marginal so I would tend leave it.
There was a problem hiding this comment.
It is too. I’m certain we can never hit this condition, but I’ll defer to the unit test.
* Invalid NSEC/3 bitmap on non-zero buffer If the PackBuffer is used to encode an NSEC/3 record, the bitmap is xored with the content of the buffer instead of being zeroed first. The algorithm has been changed so it is able zero bytes without losing too much performance (around 2x slower). * Add some comments + rename some vars to make algo clearer * Revert to previous algo with window length compute+0 on new window * Use typeBitMapLen to compute the bitmap length to zero
If the PackBuffer is used to encode an NSEC/3 record, the bitmap is
xored with the content of the buffer instead of being zeroed first.
The algorithm has been changed so it is able zero bytes without
losing too much performance (around 2x slower).