From 03bf5cda4137aca26c77ba7b7354e38f4b4e27c3 Mon Sep 17 00:00:00 2001 From: Dustin Lundquist Date: Thu, 6 Dec 2018 22:23:44 -0800 Subject: [PATCH] listener: don't use reference counting for ev_watchers 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 } --- src/listener.c | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/src/listener.c b/src/listener.c index d1dbfb0d..ce5956da 100644 --- a/src/listener.c +++ b/src/listener.c @@ -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); @@ -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); } } } @@ -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); } @@ -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); @@ -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 @@ -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) {