Skip to content

Fix uncached CLIENT_RESPONSE'es on stateful transports#208

Merged
wcawijngaards merged 2 commits intomasterfrom
bugfix/dnstap-client-response-on-stateful-transports
Mar 30, 2020
Merged

Fix uncached CLIENT_RESPONSE'es on stateful transports#208
wcawijngaards merged 2 commits intomasterfrom
bugfix/dnstap-client-response-on-stateful-transports

Conversation

@wtoorop
Copy link
Copy Markdown
Member

@wtoorop wtoorop commented Mar 30, 2020

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
Copy link
Copy Markdown
Member

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

@wcawijngaards
Copy link
Copy Markdown
Member

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
Copy link
Copy Markdown
Member Author

wtoorop commented Mar 30, 2020

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

@wcawijngaards
Copy link
Copy Markdown
Member

Okay let's merge it, then.

@wtoorop
Copy link
Copy Markdown
Member Author

wtoorop commented Mar 30, 2020

Do you want me to push the merge button?

@wcawijngaards wcawijngaards merged commit 5cb93eb into master Mar 30, 2020
wcawijngaards added a commit that referenced this pull request Mar 30, 2020
@wcawijngaards
Copy link
Copy Markdown
Member

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

Good to have this fixed.

@wcawijngaards wcawijngaards deleted the bugfix/dnstap-client-response-on-stateful-transports branch March 30, 2020 12:56
jedisct1 added a commit to jedisct1/unbound that referenced this pull request Apr 13, 2020
* 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.
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants