Bug 12510 - sock_daemon_test 4 crashes with SEGV
sock_daemon_test 4 crashes with SEGV
Status: RESOLVED FIXED
Product: Samba 4.1 and newer
Classification: Unclassified
Component: CTDB
4.6.0rc1
All All
: P5 normal
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks: 12513
  Show dependency treegraph
 
Reported: 2017-01-11 04:26 UTC by Amitay Isaacs
Modified: 2017-01-27 05:13 UTC (History)
3 users (show)

See Also:


Attachments
Patches for v4-6 (28.22 KB, patch)
2017-01-17 00:53 UTC, Amitay Isaacs
martins: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Amitay Isaacs 2017-01-11 04:26:14 UTC
This is the case of accessing memory after it has been free'd.
Comment 1 Stefan Metzmacher 2017-01-11 08:28:28 UTC
Hi Amitay,

I just noticed
https://git.samba.org/?p=amitay/samba.git;a=commitdiff;h=e63c7ab81a74598d08b454ab

I think that's not the correct fix.

There quite some layer violations (also in the current code).

sock_daemon_context_destructor() is the caller of
sock_daemon_run_send/recv and it should never call
tevent_req_done(). It should just call TALLOC_FREE(sockd->req);
Or at most it can call tevent_req_received(sockd->req).
BTW: eventd_cancel_running() seems to have a similar problem.

In the implementation of sock_daemon_run_* you're not following
the typical naming pattern for tevent_req based functions.

The state in sock_daemon_run_send should be named
sock_daemon_run_state and all step functions should
name a sock_daemon_run_ prefix. That makes it much clearer
what belongs to the flow between sock_daemon_run_send and
sock_daemon_run_recv.

In all cases you should never to anything after a tevent_req_done()
tevent_req_nomem() or tevent_req_*error*(), you have to call
'return;' (or return tevent_req_post(req, ev);) immediately.
There're quite some places where this is violated
and I guess that's the core reason for the problem.

If you really have to do more than return you need to use
tevent_req_defer_callback() to make it safe.

Using a cleanup function via tevent_req_set_cleanup_fn()
is the preferred way to trigger some cleanup code via
tevent_req_done, tevent_req_nomem, ... and tevent_req_received()
Comment 2 Amitay Isaacs 2017-01-11 09:22:48 UTC
(In reply to Stefan Metzmacher from comment #1)

Thanks for your valuable feedback.

This code has gone through many revisions, so the function names got changed around a lot and I forgot to rename the sock_daemon_start_state to sock_daemon_run_state.  I will take care of that.

I was not aware that you cannot call anything else after tevent_req_done() or tevent_req_error().  I will update the code to avoid such cases.

I really need a way to tell the sock_daemon_run computation to finish from within any of the callbacks (either sock_daemon_funcs or sock_socket_funcs).

I can think of following two options:

1. talloc_free(sockd).  This is what I have currently implemented.
   I do  understand the layer violation issue here.

2. Introduce sock_daemon_terminate(sockd)
   Would that be the right approach?

void sock_daemon_terminate(sockd)
{
    req = sockd->req;
    state = tevent_req_data(req, ...);

    sock_daemon_shutdown(state);
    sockd->req = NULL;

    tevent_req_done(req);
}

Looking at the description of tevent_req_defer_callback(), may be I need to add tevent_req_defer_callback() before calling tevent_req_done().

void sock_daemon_terminate(sockd)
{
    req = sockd->req;
    state = tevent_req_data(req, ...);

    sock_daemon_shutdown(state);
    sockd->req = NULL;

    tevent_req_defer_callback(req, state->ev);
    tevent_req_done(req);
}
Comment 3 Stefan Metzmacher 2017-01-11 09:53:53 UTC
(In reply to Amitay Isaacs from comment #2)

> I really need a way to tell the sock_daemon_run computation to finish from 
> within any of the callbacks (either sock_daemon_funcs or sock_socket_funcs).

Can't they return false or some error to indicate the termination?

If somehow possible you should try to avoid the need of the ->req pointers
completely.
Comment 4 Amitay Isaacs 2017-01-17 00:53:32 UTC
Created attachment 12833 [details]
Patches for v4-6
Comment 5 Martin Schwenke 2017-01-17 01:03:51 UTC
Hi Karolin,

This is ready for 4.6.

Thanks...
Comment 6 Karolin Seeger 2017-01-24 20:06:41 UTC
(In reply to Martin Schwenke from comment #5)
Pushed to autobuild-v4-6-test.
Comment 7 Karolin Seeger 2017-01-27 05:13:42 UTC
(In reply to Karolin Seeger from comment #6)
Pushed to v4-6-test.
Closing out bug report.

Thanks!