Skip to content

Commit

Permalink
listener: don't use reference counting for ev_watchers
Browse files Browse the repository at this point in the history
This was flagged by clang scan-build, but I'm not convinced it is even possible
since an open listener in the listeners list would always have a reference
count of at least two one for the open file descriptor and one for the
listeners list membership.

    Bug Summary
    File:	listener.c
    Warning:	line 449, column 5
    Use of memory after it is freed
    Annotated Source Code

	445	void
	446	remove_listener(struct Listener_head *listeners, struct Listener *listener, struct ev_loop *loop) {
	447	    SLIST_REMOVE(listeners, listener, Listener, entries);
	448	    close_listener(loop, listener);

    4. Calling 'close_listener'

    16. Returning; memory was released via 2nd parameter

	449	    listener_ref_put(listener);

    17. Use of memory after it is freed

	450	}

	718	static void
	719	close_listener(struct ev_loop *loop, struct Listener *listener) {
	720	    if (listener->watcher.fd < 0)

    5. Assuming the condition is false

    6. Taking false branch

	721	        return;
	722
	723	    ev_timer_stop(loop, &listener->backoff_timer);
	724	    ev_io_stop(loop, &listener->watcher);
	725	    close(listener->watcher.fd);
	726	    listener_ref_put(listener);

    7. Calling 'listener_ref_put'

    15. Returning; memory was released via 1st parameter

	727	}
	728
	729	static void
	730	free_listener(struct Listener *listener) {
	731	    if (listener == NULL)

    12. Taking false branch

	732	        return;
	733
	734	    free(listener->address);
	735	    free(listener->fallback_address);
	736	    free(listener->source_address);
	737	    free(listener->table_name);
	738
	739	    table_ref_put(listener->table);
	740	    listener->table = NULL;
	741
	742	    logger_ref_put(listener->access_log);
	743	    listener->access_log = NULL;
	744
	745	    free(listener);

    13.  Memory is released

	746	}
	747
	748	void
	749	free_listeners(struct Listener_head *listeners, struct ev_loop *loop) {
	750	    struct Listener *iter;
	751
	752	    while ((iter = SLIST_FIRST(listeners)) != NULL)

    1. Assuming the condition is true

    2. Loop condition is true.  Entering loop body

	753	        remove_listener(listeners, iter, loop);

    3. Calling 'remove_listener'

	754	}
	755
	756	/*
	757	 * Listener reference counting
	758	 *
	759	 * Since when reloading the configuration a listener with active connections
	760	 * could be removed and connections require a reference to to the listener on
	761	 * which they where received we need to allow listeners to linger outside the
	762	 * listeners list in the active configuration, and free them when their last
	763	 * connection closes.
	764	 *
	765	 * Accomplishing this with reference counting, each connection counts as a one
	766	 * reference, plus one for the active EV watchers and one for the listener
	767	 * being a member on a configurations listeners list.
	768	 */
	769	void
	770	listener_ref_put(struct Listener *listener) {
	771	    if (listener == NULL)

    8.  Taking false branch

	772	        return;
	773
	774	    assert(listener->reference_count > 0);
	775	    listener->reference_count--;
	776	    if (listener->reference_count == 0)

    9. Assuming the condition is true

    10. Taking true branch

	777	        free_listener(listener);

    11. Calling 'free_listener'

    14. Returning; memory was released via 1st parameter

	778	}
	779
	780	struct Listener *
	781	listener_ref_get(struct Listener *listener) {
	782	    listener->reference_count++;
	783	    return listener;
	784	}
  • Loading branch information
dlundquist committed Dec 7, 2018
1 parent fc5f90d commit 03bf5cd
Showing 1 changed file with 13 additions and 16 deletions.
29 changes: 13 additions & 16 deletions src/listener.c
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,9 @@ listeners_reload(struct Listener_head *existing_listeners,
display_address(new_listener->address,
address, sizeof(address)));

/* Using SLIST_REMOVE rather than remove_listener to defer
* decrementing reference count until after adding to the running
* config */
SLIST_REMOVE(new_listeners, new_listener, Listener, entries);
add_listener(existing_listeners, new_listener);
init_listener(new_listener, tables, loop);
Expand All @@ -151,11 +154,7 @@ listeners_reload(struct Listener_head *existing_listeners,
display_address(removed_listener->address,
address, sizeof(address)));

SLIST_REMOVE(existing_listeners, removed_listener, Listener, entries);
close_listener(loop, removed_listener);

/* -1 for removing from existing_listeners */
listener_ref_put(removed_listener);
remove_listener(existing_listeners, removed_listener, loop);
}
}
}
Expand Down Expand Up @@ -444,8 +443,8 @@ add_listener(struct Listener_head *listeners, struct Listener *listener) {

void
remove_listener(struct Listener_head *listeners, struct Listener *listener, struct ev_loop *loop) {
SLIST_REMOVE(listeners, listener, Listener, entries);
close_listener(loop, listener);
SLIST_REMOVE(listeners, listener, Listener, entries);
listener_ref_put(listener);
}

Expand Down Expand Up @@ -603,7 +602,6 @@ init_listener(struct Listener *listener, const struct Table_head *tables,
ev_io_init(&listener->watcher, accept_cb, sockfd, EV_READ);
listener->watcher.data = listener;
listener->backoff_timer.data = listener;
listener_ref_get(listener);

ev_io_start(loop, &listener->watcher);

Expand Down Expand Up @@ -717,13 +715,13 @@ print_listener_config(FILE *file, const struct Listener *listener) {

static void
close_listener(struct ev_loop *loop, struct Listener *listener) {
if (listener->watcher.fd < 0)
return;

ev_timer_stop(loop, &listener->backoff_timer);
ev_io_stop(loop, &listener->watcher);
close(listener->watcher.fd);
listener_ref_put(listener);

if (listener->watcher.fd >= 0) {
ev_io_stop(loop, &listener->watcher);
close(listener->watcher.fd);
listener->watcher.fd = -1;
}
}

static void
Expand Down Expand Up @@ -762,9 +760,8 @@ free_listeners(struct Listener_head *listeners, struct ev_loop *loop) {
* listeners list in the active configuration, and free them when their last
* connection closes.
*
* Accomplishing this with reference counting, each connection counts as a one
* reference, plus one for the active EV watchers and one for the listener
* being a member on a configurations listeners list.
* Accomplishing this with reference counting: membership in a Config listener
* list counts as one as does each connection.
*/
void
listener_ref_put(struct Listener *listener) {
Expand Down

0 comments on commit 03bf5cd

Please sign in to comment.