BUG/MEDIUM: deviceatlas: isolate threads while swapping the reloaded atlas

When da_haproxy_checkinst() hot-reloads the atlas, the old instance was
closed and its image freed under dadwsch_lock only, while lookups on other
threads take no lock. With several threads, a request could search a freed
image or a partially copied atlas and crash. In addition, the other threads
spun on the lock during the whole compilation of the new data file, long
enough for the watchdog to kill the process with large files.

The lock is now only tried, so other threads keep serving with the current
atlas during the compilation. Once the new atlas is ready the lock is
released, the swap is done between thread_isolate() and thread_release(),
and the old image is freed afterwards.

This should be backported to lower branches.

v2: release dadwsch_lock before thread_isolate(), which must not be
called with a lock held.

diff --git a/addons/deviceatlas/da.c b/addons/deviceatlas/da.c
index 55429a7c54632b7a235138eda502d892038413bd..157ebb11023429d0de94598a1306f85500e63832 100644 (file)
--- a/addons/deviceatlas/da.c
+++ b/addons/deviceatlas/da.c
@@ -6,6 +6,7 @@
 #include <haproxy/api.h>
 #include <haproxy/arg.h>
 #include <haproxy/cfgparse.h>
+#include <haproxy/clock.h>
 #include <haproxy/errors.h>
 #include <haproxy/global.h>
 #include <haproxy/http.h>
@@ -287,14 +288,15 @@ static void da_haproxy_checkinst(void)
 
             if (base[0] != 0) {
             FILE *jsonp;
-            void *cnew;
+            void *cnew, *cold;
             da_status_t status;
             size_t atlassz;
             char atlasp[ATLASTOKSZ] = {0};
             da_atlas_t inst;
             da_property_decl_t extraprops[1] = {{NULL, 0}};
 #ifdef USE_THREAD
-            HA_SPIN_LOCK(OTHER_LOCK, &dadwsch_lock);
+            if (HA_SPIN_TRYLOCK(OTHER_LOCK, &dadwsch_lock) != 0)
+                return;
             if (base[0] == 0) {
                 HA_SPIN_UNLOCK(OTHER_LOCK, &dadwsch_lock);
                 return;
@@ -316,23 +318,29 @@ static void da_haproxy_checkinst(void)
             fclose(jsonp);
             if (status == DA_OK) {
                 if (da_atlas_open(&inst, extraprops, cnew, atlassz) == DA_OK) {
+                    size_t hi, maxhdrlen = 16;
+
                     inst.config.cache_size = global_deviceatlas.cachesize;
+                    for (hi = 0; hi < inst.header_evidence_count; hi++) {
+                        size_t nl = strlen(inst.header_priorities[hi].name);
+                        if (nl > maxhdrlen)
+                            maxhdrlen = nl;
+                    }
+                    base[0] = 0;
+#ifdef USE_THREAD
+                    HA_SPIN_UNLOCK(OTHER_LOCK, &dadwsch_lock);
+#endif
+                    thread_isolate();
                     da_atlas_close(&global_deviceatlas.atlas);
-                    free(global_deviceatlas.atlasimgptr);
+                    cold = global_deviceatlas.atlasimgptr;
                     global_deviceatlas.atlasimgptr = cnew;
                     global_deviceatlas.atlas = inst;
-                    {
-                        size_t hi;
-                        global_deviceatlas.maxhdrlen = 16;
-                        for (hi = 0; hi < inst.header_evidence_count; hi++) {
-                            size_t nl = strlen(inst.header_priorities[hi].name);
-                            if (nl > global_deviceatlas.maxhdrlen)
-                                global_deviceatlas.maxhdrlen = nl;
-                        }
-                    }
-                    base[0] = 0;
+                    global_deviceatlas.maxhdrlen = maxhdrlen;
+                    thread_release();
+                    free(cold);
                     ha_notice("deviceatlas : new instance, data file date `%s`.\n",
                         da_getdatacreationiso8601(&global_deviceatlas.atlas));
+                    return;
                 } else {
                     ha_alert("deviceatlas : instance update failed.\n");
                     free(cnew);