Skip to content
Snippets Groups Projects

improve TLS error handling

Merged improve TLS error handling
tls-crash into master
1 unresolved thread
Merged Grigorii Demidov requested to merge tls-crash into master
1 unresolved thread

Closes #340

Edited by Petr Špaček

Merge request reports

Pipeline #35374 passed with warnings

Stage: build
Stage: test
Stage: coverage

Pipeline passed with warnings for d9350443 on tls-crash

Test coverage 71.70% (5.90%) from 1 job
Approval is optional

Merged by Petr ŠpačekPetr Špaček 6 years ago (Apr 13, 2018 7:32am UTC)

Merge details

  • Changes merged into with a1f3feef.
  • Deleted the source branch.

Pipeline #35375 passed with warnings

Stage: build
Stage: test
Stage: coverage
Stage: deploy

Pipeline passed with warnings for a1f3feef on master

Test coverage 71.70% (5.90%) from 1 job

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
daemon/worker.c
1478 return state == KR_STATE_DONE ? 0 : kr_error(EIO);
1492 1479 }
1480
1481 /* Send back answer */
1482 struct session *source_session = ctx->source.session;
1483 uv_handle_t *handle = source_session->handle;
1484 assert(source_session->closing == false);
1485 assert(handle && handle->data == ctx->source.session);
1486 assert(ctx->source.addr.ip.sa_family != AF_UNSPEC);
1487 int res = qr_task_send(task, handle,
1488 (struct sockaddr *)&ctx->source.addr,
1489 ctx->req.answer);
1490 if (res != kr_ok()) {
1491 while (source_session->tasks.len > 0) {
1492 struct qr_task *t = source_session->tasks.at[0];
1493 (void) qr_task_on_send(t, NULL, kr_error(EIO));
  • Marek Vavrusa
    Marek Vavrusa @vavrusam ·
    Reporter

    I guess session_del_tasks is missing here?

  • Marek Vavrusa
    Marek Vavrusa @vavrusam ·
    Reporter

    It passes NULL handle, so it will never reach the session_del_tasks inside qr_task_on_send. I guess this is a copypasta error, as the same statement sequence was copied in multiple places in a8f2f543

  • Marek Vavrusa
    Marek Vavrusa @vavrusam ·
    Reporter

    @gdemidov Maybe you wanted to pass the handle here, that would hopefully get to the code path that removes the task from session.

  • Marek Vavrusa
    Marek Vavrusa @vavrusam ·
    Reporter

    You can reproduce it by starting 2+ queries over the same connection and closing it (or change the res != kr_ok() condition to true to always cancel queries. The first one will be removed from the sesssion task list because it's finished, the rest of the queries are not, and the handle is NULL, so it will loop indefinitely:

    $ cat queries.dat
navy.mil A
www.navy.mil A
www.nrl.navy.mil A
$ getdns_query @127.0.0.1#8053 -a -s -L -B -F queries.dat
  • Marek Vavrusa
    Marek Vavrusa @vavrusam ·
    Reporter

    So passing handle doesn't help, removing the task causes an assertion failure elsewhere:

    Assertion failed: (handle && handle->data == ctx->source.session), function qr_task_finalize, file daemon/worker.c, line 1539.
Abort trap: 6
  • Marek Vavrusa
    Marek Vavrusa @vavrusam ·
    Reporter

    I think the correct solution is just to call qr_task_on_send on current task when send fails, and let the tasks on the same connection complete and then fail on send, and so naturally draining the task list. The cancellation here gets difficult when other tasks on the same connection have pending subrequests and timers.

  • Marek Vavrusa
    Marek Vavrusa @vavrusam ·
    Reporter

    https://gitlab.labs.nic.cz/knot/knot-resolver/merge_requests/559

  • Please register or sign in to reply
  • Petr Špaček added tests label

    added tests label

  • Petr Špaček removed plan tests labels

    removed plan tests labels

  • Petr Špaček added feature label

    added feature label

    • Please register or sign in to reply