BUG/MEDIUM: hlua_fcn: ensure systematic watcher cleanup for server list iterator

In 358166a ("BUG/MINOR: hlua_fcn: restore server pairs iterator pointer
consistency"), I wrongly assumed that because the iterator was a temporary
object, no specific cleanup was needed for the watcher.

In fact watcher_detach() is not only relevant for the watcher itself, but
especially for its parent list to remove the current watcher from it.

As iterators are temporary objects, failing to remove their watchers from
the server watcher list causes the server watcher list to be corrupted.

On a normal iteration sequence, the last watcher_next() receives NULL
as target so it successfully detaches the last watcher from the list.
However the corner case here is with interrupted iterators: users are
free to break away from the iteration loop when a specific condition is
met for instance from the lua script, when this happens
hlua_listable_servers_pairs_iterator() doesn't get a chance to detach the
last iterator.

Also, Lua doesn't tell us that the loop was interrupted,
so to fix the issue we rely on the garbage collector to force a last
detach right before the object is freed. To achieve that, watcher_detach()
was slightly modified so that it becomes possible to call it without
knowing if the watcher is already detached or not, if watcher_detach() is
called on a detached watcher, the function does nothing. This way it saves
the caller from having to track the watcher state and makes the API a
little more convenient to use. This way we now systematically call
watcher_detach() for server iterators right before they are garbage
collected.

This was first reported in GH #3055. It can be observed when the server
list is browsed one than more time when it was already browsed from Lua
for a given proxy and the iteration was interrupted before the end. As the
watcher list is corrupted, the common symptom is watcher_attach() or
watcher_next() not ending due to the internal mt_list call looping
forever.

Thanks to GH users @sabretus and @sabretus for their precious help.

It should be backported everywhere 358166a was.

include/haproxy/list.h

@@ -284,10 +284,11 @@ static __inline void watcher_attach(struct watcher *w, void *target)
 		MT_LIST_APPEND(list, &w->el);
 }
 
-/* Untracks target via <w> watcher. Invalid if <w> is not attached first. */
+/* Untracks target via <w> watcher. Does nothing if <w> is not attached */
 static __inline void watcher_detach(struct watcher *w)
 {
-	BUG_ON_HOT(!MT_LIST_INLIST(&w->el));
+	if (!MT_LIST_INLIST(&w->el))
+		return;
 	*w->pptr = NULL;
 	MT_LIST_DELETE(&w->el);
 }

src/hlua_fcn.c

@@ -1913,6 +1913,21 @@ int hlua_listable_servers_pairs_iterator(lua_State *L)
 	return 2;
 }
 
+/* ensure proper cleanup for listable_servers_pairs */
+int hlua_listable_servers_pairs_gc(lua_State *L)
+{
+	struct hlua_server_list_iterator_context *ctx;
+
+	ctx = lua_touserdata(L, 1);
+
+	/* we need to make sure that the watcher leaves in detached state even
+	 * if the iterator was interrupted (ie: "break" from the loop), else
+	 * the server watcher list will become corrupted
+	 */
+	watcher_detach(&ctx->srv_watch);
+	return 0;
+}
+
 /* init the iterator context, return iterator function
  * with context as closure. The only argument is a
  * server list object.
@@ -1925,6 +1940,12 @@ int hlua_listable_servers_pairs(lua_State *L)
 	hlua_srv_list = hlua_check_server_list(L, 1);
 
 	ctx = lua_newuserdata(L, sizeof(*ctx));
+
+	/* add gc metamethod to the newly created userdata */
+	lua_newtable(L);
+	hlua_class_function(L, "__gc", hlua_listable_servers_pairs_gc);
+	lua_setmetatable(L, -2);
+
 	ctx->px = hlua_srv_list->px;
 	ctx->next = NULL;
 	watcher_init(&ctx->srv_watch, &ctx->next, offsetof(struct server, watcher_list));