dnsdist: Fix a double-free when a DoH cross-protocol response is dropped

[rgacogne]

Short description

Introduced in 1.7.0, and only when a cross-protocol response to a DoH query is dropped by a rule.

Checklist

I have:

• read the CONTRIBUTING.md document
• compiled this code
• tested this code
• included documentation (including possible behaviour changes)
• documented the code
• added or modified regression test(s)
• added or modified unit test(s)

[omoerbeek]

This change looks good.

I checked the places where DoHTCPCrossQuerySender and DoHCrossProtocolQuery objects are used, and noted one spot where an DoHTCPCrossQuerySender object is first created and then the get() is done: line 1328. I do not think it is wrong, but the pattern looked strange to me.

[rgacogne]

Thanks Otto! I'll review all the places where we touch the reference counter to check that we are doing the right thing. I'll also check one more time if we cannot use a smart pointer instead but h2o's API make that hard, if not impossible.

[rgacogne]

It was indeed leaking when a UDP response to a DoH query was truncated. I pushed three commits:

1. fixing that leak
2. making it easier to understand which function has ownership of the object
3. wrapping the object in a unique_ptr whenever possible, reducing the amount of manual book-keeping

[rgacogne]

2171e7c does not play well with TSAN, investigating.

[rgacogne]

Fixed for now, it looks like TSAN is confused by the fact that the answer from the backend might come back in a UDP responder thread before the send() syscall returned in the initial thread.

[omoerbeek]

In doh.cc:

/* you can't touch du after this line, because it might already have been freed */

still du is accessed in the block below (this is not part of this PR but it caught my eye anyway). Either the comment should be adapted or something else is going on.

[rgacogne]

The comment should say that du can't be touched if that call returns a non-negative value, right?

[omoerbeek]

Some small remarks. Ref counting is inherently a bit of a scary subject as it is easy to get wrong, but I do think this approach is better than before.

[omoerbeek]

Suggestion: use duNaked or something like that to make it very clear it's not a smart pointer when using it as other instances of du hold a unique_pointer.

[rgacogne]

Thanks, Otto! I pushed a few commits following your advice, I'm still pondering the duNaked one, I can't decide whether I like that idea.

[omoerbeek]

The comment should say that du can't be touched if that call returns a non-negative value, right?

yes, that is more clear

[rgacogne]

I'm going to merge this without changing the duNaked stuff because I'd like to release rc1 quickly, and also because I hope to be able to use unique_ptrs everywhere in 1.8.0 anyway.