From 0eac509d69aa4b3b811fd95f2e2fe115a5aac663 Mon Sep 17 00:00:00 2001 From: Bradley Corrion Date: Fri, 21 Aug 2026 10:08:49 -0600 Subject: [PATCH 1/2] Fix bvlc_register_with_bbmd() reporting FAILED on real success dlenv_register_as_foreign_device() runs synchronously from main(), before the receive loop starts, so bvlc_register_with_bbmd()'s pthread_cond_timedwait() was blocking on a signal that can only ever come from bvlc_handler() processing a BVLC_RESULT -- and bvlc_handler() only ever runs as a side effect of bip_receive(), which is only ever called from the main receive loop that hasn't started yet. Nothing else reads the socket during the wait (bacnet-sim's only other thread is the unrelated Lua script runner, started later), so the wait could never observe the BBMD's real ACK -- it could only time out and report failure, even when registration genuinely succeeded on the wire. Confirmed against a real BBMD (iotech-mini-bbmd): its own log showed the registration being received and processed correctly while bacnet-sim simultaneously self-reported "FAILED to Register with BBMD". Fix: poll bip_receive() directly during the wait instead of relying on a condition variable nothing can signal in time. bip_receive() already invokes bvlc_handler() internally as a side effect, so this correctly observes the real ACK/NAK. Verified with a rebuilt bacnet-sim binary against a live mini-bbmd: now logs "SUCCEEDED to Register with BBMD", with mini-bbmd's own log independently confirming the same registration event from the exact matching source address at the same timestamp. Co-Authored-By: Claude Sonnet 5 --- src/bacnet/basic/bbmd/h_bbmd.c | 33 +++++++++++++++++++++------------ 1 file changed, 21 insertions(+), 12 deletions(-) diff --git a/src/bacnet/basic/bbmd/h_bbmd.c b/src/bacnet/basic/bbmd/h_bbmd.c index 4230f592f3..8b58a123fa 100644 --- a/src/bacnet/basic/bbmd/h_bbmd.c +++ b/src/bacnet/basic/bbmd/h_bbmd.c @@ -38,6 +38,7 @@ #include /* for memcpy */ #include #include +#include #include "bacnet/bacdcode.h" #include "bacnet/datalink/bip.h" #include "bacnet/datalink/bvlc.h" @@ -1167,32 +1168,40 @@ int bvlc_register_with_bbmd(BACNET_IP_ADDRESS *bbmd_addr, uint16_t ttl_seconds) bbmd_reg = BBMD_REG_UNSET; pthread_mutex_unlock (&mutex); - /* Setup a 30 second condition variable wait */ + /* bvlc_handler() still calls pthread_cond_signal(&cond) on receipt of + * a BVLC_RESULT (see below) -- kept initialized even though nothing + * waits on it anymore below, since a signal on an uninitialized + * condvar would be undefined behavior. */ pthread_cond_init (&cond, NULL); - time_t timeout_seconds = 3; - struct timeval now; - struct timespec timeout; - int timeout_count = 0; int retval = bip_send_mpdu(bbmd_addr, &BVLC_Buffer[0], BVLC_Buffer_Len); if (retval == -1) { return retval; } - while (timeout_count < 10) + + /* Nothing else reads the BIP socket at this point in startup -- the + * main receive loop (bip_receive() via datalink_receive()) hasn't + * started yet, so bvlc_handler() can never run to set bbmd_reg unless + * THIS call also pumps the socket itself. A pthread_cond_timedwait() + * here (the original approach) can never be signaled: nothing else in + * this process reads the BBMD's real ACK/NAK off the wire during the + * wait, so it always timed out and reported failure even when the + * BBMD's registration genuinely succeeded. Poll bip_receive() + * directly instead -- it invokes bvlc_handler() internally on any + * packet received, which is what actually sets bbmd_reg. */ + BACNET_ADDRESS bbmd_reply_src = { 0 }; + uint8_t bbmd_reply_mtu[MAX_MPDU] = { 0 }; + time_t deadline = time(NULL) + 30; + while (time(NULL) < deadline) { - gettimeofday (&now, NULL); - timeout.tv_sec = now.tv_sec + timeout_seconds; - timeout.tv_nsec = 0; + bip_receive(&bbmd_reply_src, bbmd_reply_mtu, sizeof(bbmd_reply_mtu), 100); pthread_mutex_lock (&mutex); - pthread_cond_timedwait (&cond, &mutex, &timeout); bool received_response = bbmd_reg != BBMD_REG_UNSET; pthread_mutex_unlock (&mutex); if (received_response) break; - timeout_count++; } - pthread_cond_destroy(&cond); pthread_mutex_destroy(&mutex); /* Fail if the BBMD registration was not successful */ From ca0beae0708b635a641df3646801db0b0b7f2680 Mon Sep 17 00:00:00 2001 From: Bradley Corrion Date: Fri, 21 Aug 2026 14:26:20 -0600 Subject: [PATCH 2/2] Address review feedback: statically initialize mutex/cond bvlc_handler() locks/signals mutex/cond unconditionally on any incoming BVLC_RESULT, including one that arrives before bvlc_register_with_bbmd() is ever called for the first time -- using them before pthread_mutex_init()/pthread_cond_init() ran would be undefined behavior. Separately, the early return on bip_send_mpdu() failure skipped pthread_mutex_destroy(), leaking the mutex on that path. Switch mutex/cond to static initializers (PTHREAD_MUTEX_INITIALIZER/ PTHREAD_COND_INITIALIZER) instead of runtime init/destroy -- they're now always valid for the life of the process, which resolves both issues at once and matches how BVLC_RESULT's handler already treats them as always-available globals. Re-verified live against production mini-bbmd after this change: bacnet-sim still logs "SUCCEEDED to Register with BBMD", with mini-bbmd's own log independently confirming the same registration event. Co-Authored-By: Claude Sonnet 5 --- src/bacnet/basic/bbmd/h_bbmd.c | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/src/bacnet/basic/bbmd/h_bbmd.c b/src/bacnet/basic/bbmd/h_bbmd.c index 8b58a123fa..1582bf7ade 100644 --- a/src/bacnet/basic/bbmd/h_bbmd.c +++ b/src/bacnet/basic/bbmd/h_bbmd.c @@ -101,9 +101,16 @@ typedef enum BBMD_REG_SUCCESS } bbmd_reg_t; -/** Mutex and condition variable for checking if BBMD registration has been successful */ -static pthread_mutex_t mutex; -static pthread_cond_t cond; +/** Mutex and condition variable for checking if BBMD registration has been + * successful. Statically initialized (not pthread_*_init()'d at runtime) + * because the BVLC_RESULT handler below locks/signals them unconditionally + * on any incoming BVLC_RESULT, including one that arrives before + * bvlc_register_with_bbmd() is ever called -- using them before a runtime + * init would be undefined behavior otherwise. Never destroyed, for the + * same reason: they must stay valid for the life of the process, not just + * for the duration of one registration attempt. */ +static pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER; +static pthread_cond_t cond = PTHREAD_COND_INITIALIZER; static bbmd_reg_t bbmd_reg; /** @@ -1161,19 +1168,11 @@ int bvlc_register_with_bbmd(BACNET_IP_ADDRESS *bbmd_addr, uint16_t ttl_seconds) BVLC_Buffer_Len = bvlc_encode_register_foreign_device( &BVLC_Buffer[0], sizeof(BVLC_Buffer), ttl_seconds); - pthread_mutex_init (&mutex, NULL); - pthread_mutex_lock (&mutex); /* Set the initial value of the BBMD registration bool to false */ bbmd_reg = BBMD_REG_UNSET; pthread_mutex_unlock (&mutex); - /* bvlc_handler() still calls pthread_cond_signal(&cond) on receipt of - * a BVLC_RESULT (see below) -- kept initialized even though nothing - * waits on it anymore below, since a signal on an uninitialized - * condvar would be undefined behavior. */ - pthread_cond_init (&cond, NULL); - int retval = bip_send_mpdu(bbmd_addr, &BVLC_Buffer[0], BVLC_Buffer_Len); if (retval == -1) { @@ -1202,8 +1201,6 @@ int bvlc_register_with_bbmd(BACNET_IP_ADDRESS *bbmd_addr, uint16_t ttl_seconds) if (received_response) break; } - pthread_mutex_destroy(&mutex); - /* Fail if the BBMD registration was not successful */ if (bbmd_reg != BBMD_REG_SUCCESS) {