From 0eac509d69aa4b3b811fd95f2e2fe115a5aac663 Mon Sep 17 00:00:00 2001 From: Bradley Corrion Date: Fri, 21 Aug 2026 10:08:49 -0600 Subject: [PATCH 1/3] 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/3] 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) { From 61178548d4a0622bbc8ad616de045ef757ad2707 Mon Sep 17 00:00:00 2001 From: Bradley Corrion Date: Tue, 22 Sep 2026 10:21:01 -0600 Subject: [PATCH 3/3] Address review feedback: only self-poll bip_receive() as a fallback bip_receive() polling directly competes with any other thread already reading this socket for whichever datagram the OS hands to whichever caller asks first -- a caller whose own receive thread is already running before it registers (e.g. device-bacnet-c-private, which starts receive_data() before calling register_as_foreign_device()) could have this loop win a real incoming message instead of its normal thread, silently dropping it (only bvlc_handler() runs on it here, never npdu_handler()). Split the wait into two phases instead: first, a short (2s) bounded pthread_cond_timedwait() using the original signal mechanism -- if something else is already pumping the socket, this is signaled almost immediately and phase two never runs, so behavior for a caller like device-bacnet-c-private is unchanged from before this fix existed. Only fall back to self-polling bip_receive() once that grace period elapses with nothing having signaled us, which is the signature of a caller with no other thread servicing the socket yet (bacnet-sim's case, calling this synchronously from main() before its receive loop starts). Total worst-case wait is unchanged at 30s. Verified against a real running mini-bbmd: a bacnet-sim built from this patched stack still logs "SUCCEEDED to Register with BBMD" (previously "FAILED" before either fix), independently confirmed by mini-bbmd's own log showing the registration received and processed. Co-Authored-By: Claude Sonnet 5 --- src/bacnet/basic/bbmd/h_bbmd.c | 71 ++++++++++++++++++++++++++-------- 1 file changed, 54 insertions(+), 17 deletions(-) diff --git a/src/bacnet/basic/bbmd/h_bbmd.c b/src/bacnet/basic/bbmd/h_bbmd.c index 1582bf7ade..13e100119b 100644 --- a/src/bacnet/basic/bbmd/h_bbmd.c +++ b/src/bacnet/basic/bbmd/h_bbmd.c @@ -1179,26 +1179,63 @@ int bvlc_register_with_bbmd(BACNET_IP_ADDRESS *bbmd_addr, uint16_t ttl_seconds) return retval; } - /* 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) + bool received_response = false; + time_t start = time(NULL); + time_t deadline = start + 30; + + /* Phase 1: give whatever's already reading this socket -- e.g. a + * caller's own pre-existing receive thread, such as + * device-bacnet-c-private's receive_data(), which starts before it + * ever registers -- the normal signal path's chance to deliver the + * reply first. As long as something else is already pumping + * bip_receive() in a loop, that path works correctly and never + * competes with it for datagrams on the socket, exactly as it always + * has. A short, bounded grace period is enough: on a real network an + * already-running receive thread signals within well under a second + * of the BBMD's reply arriving. */ + time_t grace_deadline = start + 2; + while (!received_response && time(NULL) < grace_deadline) { - bip_receive(&bbmd_reply_src, bbmd_reply_mtu, sizeof(bbmd_reply_mtu), 100); + struct timeval now; + struct timespec timeout; + gettimeofday (&now, NULL); + timeout.tv_sec = now.tv_sec; + timeout.tv_nsec = (now.tv_usec + 250000) * 1000; + if (timeout.tv_nsec >= 1000000000) + { + timeout.tv_sec += 1; + timeout.tv_nsec -= 1000000000; + } pthread_mutex_lock (&mutex); - bool received_response = bbmd_reg != BBMD_REG_UNSET; + pthread_cond_timedwait (&cond, &mutex, &timeout); + received_response = bbmd_reg != BBMD_REG_UNSET; pthread_mutex_unlock (&mutex); - if (received_response) break; + } + + /* Phase 2: nobody signaled us within the grace period, which is the + * signature of a caller -- like bacnet-sim, which calls this + * synchronously from main() before starting any receive loop -- that + * has no other thread pumping the socket and so no way to ever be + * signaled by the normal path. Only now poll bip_receive() ourselves: + * it invokes bvlc_handler() internally on any packet received, which + * is what actually sets bbmd_reg. This does compete with any other + * reader of the socket for whichever datagram the OS hands to + * whichever caller asks first -- a real risk in general, since the + * loser silently drops a datagram that never reaches bvlc_handler() + * for anything but this fallback's own narrow use, but fine here + * because phase 1 already established nothing else is claiming + * datagrams from this socket. */ + if (!received_response) + { + BACNET_ADDRESS bbmd_reply_src = { 0 }; + uint8_t bbmd_reply_mtu[MAX_MPDU] = { 0 }; + while (!received_response && time(NULL) < deadline) + { + bip_receive(&bbmd_reply_src, bbmd_reply_mtu, sizeof(bbmd_reply_mtu), 100); + pthread_mutex_lock (&mutex); + received_response = bbmd_reg != BBMD_REG_UNSET; + pthread_mutex_unlock (&mutex); + } } /* Fail if the BBMD registration was not successful */