Fix uncached CLIENT_RESPONSE'es on stateful transports #208

wtoorop · 2020-03-30T09:56:40Z

I noticed dnstap CLIENT_RESPONSE did not contain the response sent to the client when:

the client is connected on a stateful transport, and
the response did not come from cache.

When investigating the issue, I noticed that repinfo->c->buffer did not contain the response in this case (second half of comm_point_send_reply).
Only after tcp_req_info_send_reply() was called, the the buffer contains the response.

Is it safe to change this order?

Because repinfo->c->buffer does not contain the response when the it did not came from cache. Only after tcp_req_info_send_reply is called, is the response on the buffer which is used to fill the dnstap protobuf's.

wcawijngaards · 2020-03-30T09:59:05Z

No, it may be somewhere else, in a queue.

wcawijngaards · 2020-03-30T10:04:16Z

I think this may be better. Also buffer may no longer be there, because allocated in queue for tcp responses.

index 09023838..049c7ff0 100644
--- a/util/netevent.c
+++ b/util/netevent.c
@@ -3157,7 +3157,7 @@ comm_point_send_reply(struct comm_reply *repinfo)
                if(repinfo->c->tcp_parent->dtenv != NULL &&
                   repinfo->c->tcp_parent->dtenv->log_client_response_messages)
                        dt_msg_send_client_response(repinfo->c->tcp_parent->dtenv,
-                       &repinfo->addr, repinfo->c->type, repinfo->c->buffer);
+                       &repinfo->addr, repinfo->c->type, (repinfo->c->tcp_req_info?repinfo->c->tcp_req_info->spool_buffer:repinfo->c->buffer));
 #endif
                if(repinfo->c->tcp_req_info) {
                        tcp_req_info_send_reply(repinfo->c->tcp_req_info);

When tcp_req_info exists. This fixes that dnstap CLIENT_RESPONSE messages did not contain the response message when answering on statful transport for uncached responses.

wtoorop · 2020-03-30T10:21:52Z

Yes that works! Thanks! I've done a new commit with your proposed fix.

wcawijngaards · 2020-03-30T10:51:40Z

Okay let's merge it, then.

wtoorop · 2020-03-30T12:40:16Z

Do you want me to push the merge button?

transports.

wcawijngaards · 2020-03-30T12:55:59Z

Sure! I don't know who should merge, but I just did.

Good to have this fixed.

* nlnet/master: (30 commits) - Merge PR NLnetLabs#214 from gearnode: unbound-control-setup recreate certificates. With the -r option the certificates are created again, without it, only the files that do not exist are created. fix unbound-control-setup is not idempotent - Keep track of number of timeouts. Use this counter to determine if capsforid fallback should be started. - More documentation for redis-expire-records option. - Changes for PR NLnetLabs#206 (formatting and remade lex and yacc output). changed init logic of redis backend as per review request implemented review feedback renamed option from 'redis-set-ttl' to 'redis-expire-records' added option 'redis-set-ttl' to define whether ttl should be added to redis records added check for redis command 'setex' when initializing redis connection updated documentation minor improvements to previous changes - Merge PR NLnetLabs#208: Fix uncached CLIENT_RESPONSE'es on stateful transports. Send tcp_req_info->spool_buffer as dnstap CLIENT_RESPONSE Fix uncached CLIENT_RESPONSE'es on stateful transports nroff fix for dash. - Merge PR NLnetLabs#207: Clarify if-automatic listens on 0.0.0.0 and :: Clarify if-automatic listens on 0.0.0.0 and :: honor 'server_expired_ttl' in redis added logic for redis to honor ttl when serve_expired is not enabled Changelog note for PR NLnetLabs#203. - Merge PR NLnetLabs#203 from noloader: Update README-Travis.md with current procedures. Make unbound-control error returned on missing domain name more user friendly. Update README-Travis.md with current procedures - Fix RPZ concurrency issue when using auth_zone_reload. ...

Fix uncached CLIENT_RESPONSE'es on stateful transports

9d9eee8

Because repinfo->c->buffer does not contain the response when the it did not came from cache. Only after tcp_req_info_send_reply is called, is the response on the buffer which is used to fill the dnstap protobuf's.

Send tcp_req_info->spool_buffer as dnstap CLIENT_RESPONSE

af0bd5b

When tcp_req_info exists. This fixes that dnstap CLIENT_RESPONSE messages did not contain the response message when answering on statful transport for uncached responses.

wcawijngaards merged commit 5cb93eb into master Mar 30, 2020

wcawijngaards added a commit that referenced this pull request Mar 30, 2020

- Merge PR #208: Fix uncached CLIENT_RESPONSE'es on stateful

94e92b1

transports.

wcawijngaards deleted the bugfix/dnstap-client-response-on-stateful-transports branch March 30, 2020 12:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Fix uncached CLIENT_RESPONSE'es on stateful transports #208

Fix uncached CLIENT_RESPONSE'es on stateful transports #208

Uh oh!

wtoorop commented Mar 30, 2020 •

edited

Loading

Uh oh!

wcawijngaards commented Mar 30, 2020

Uh oh!

wcawijngaards commented Mar 30, 2020

Uh oh!

wtoorop commented Mar 30, 2020

Uh oh!

wcawijngaards commented Mar 30, 2020

Uh oh!

wtoorop commented Mar 30, 2020

Uh oh!

wcawijngaards commented Mar 30, 2020

Uh oh!

Uh oh!

Uh oh!

Fix uncached CLIENT_RESPONSE'es on stateful transports #208

Fix uncached CLIENT_RESPONSE'es on stateful transports #208

Uh oh!

Conversation

wtoorop commented Mar 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wcawijngaards commented Mar 30, 2020

Uh oh!

wcawijngaards commented Mar 30, 2020

Uh oh!

wtoorop commented Mar 30, 2020

Uh oh!

wcawijngaards commented Mar 30, 2020

Uh oh!

wtoorop commented Mar 30, 2020

Uh oh!

wcawijngaards commented Mar 30, 2020

Uh oh!

Uh oh!

wtoorop commented Mar 30, 2020 •

edited

Loading