There is a hash bug when the daqd computes the channel hash to send the to run number server.

It truncates the name/units.  This code adds an updated hash function that correctly uses the name/units.  It keeps the old function availble for use when a parameter is set for compatibility.

src/daqd/CMakeLists.txt

@@ -157,10 +157,13 @@ add_executable(test_daqd_unit_tests tests/test_main.cc
 		tests/test_daqd_cmask_t.cc
 		tests/test_shmem_recv.cc
 		tests/test_daqd_thread.cc
+		tests/test_daqd_channel_hashes.cc
         daqd_thread.cc
 		)
 target_include_directories(test_daqd_unit_tests PUBLIC ${CMAKE_CURRENT_SOURCE_DIR} ../include)
-target_link_libraries(test_daqd_unit_tests PUBLIC ${CMAKE_THREAD_LIBS_INIT})
+target_link_libraries(test_daqd_unit_tests PUBLIC
+		ldastools::framecpp
+		${CMAKE_THREAD_LIBS_INIT})
 add_test(NAME test_daqd_unit_tests
 		COMMAND test_daqd_unit_tests
 		WORKING_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}")

src/daqd/channel.hh

@@ -3,6 +3,7 @@
 
 #include "channel.h"
 #include <string.h>
+#include <cstdlib>
 
 typedef struct
 {
@@ -129,6 +130,51 @@ public:
     }
 };
 
+template < typename Hash >
+class FrameCpp_hash_adapter
+{
+public:
+    explicit FrameCpp_hash_adapter( Hash& hash ) : hash_{ hash }
+    {
+    }
+    FrameCpp_hash_adapter( const FrameCpp_hash_adapter& ) = default;
+
+    void
+    add( const void* data, std::size_t length )
+    {
+        // why is this not const void* ?
+        hash_.Update( const_cast< void* >( data ), length );
+    }
+
+private:
+    Hash& hash_;
+};
+
+template < typename HashObject >
+void
+hash_channel_v0_broken( HashObject& hash, const channel_t& channel )
+{
+    // this is broken, but is kept (for now) for compatibility
+    // the name and units sum the first sizeof(size_t) bytes of their values
+    hash.add( &( channel.chNum ), sizeof( channel.chNum ) );
+    hash.add( &( channel.seq_num ), sizeof( channel.seq_num ) );
+    size_t name_len = strnlen( channel.name, channel_t::channel_name_max_len );
+    hash.add( channel.name, sizeof( name_len ) );
+    hash.add( &( channel.sample_rate ), sizeof( channel.sample_rate ) );
+    hash.add( &( channel.active ), sizeof( channel.active ) );
+    hash.add( &( channel.trend ), sizeof( channel.trend ) );
+    hash.add( &( channel.group_num ), sizeof( channel.group_num ) );
+    hash.add( &( channel.bps ), sizeof( channel.bps ) );
+    hash.add( &( channel.dcu_id ), sizeof( channel.dcu_id ) );
+    hash.add( &( channel.data_type ), sizeof( channel.data_type ) );
+    hash.add( &( channel.signal_gain ), sizeof( channel.signal_gain ) );
+    hash.add( &( channel.signal_slope ), sizeof( channel.signal_slope ) );
+    hash.add( &( channel.signal_offset ), sizeof( channel.signal_offset ) );
+    size_t unit_len =
+        strnlen( channel.signal_units, channel_t::engr_unit_max_len );
+    hash.add( channel.signal_units, sizeof( unit_len ) );
+}
+
 template < typename HashObject >
 void
 hash_channel( HashObject& hash, const channel_t& channel )

src/daqd/daqd.cc

@@ -501,32 +501,27 @@ daqd_c::update_configuration_number( const char* source_address )
     if ( num_channels == 0 )
         return;
 
-    FrameCPP::Common::MD5Sum check_sum;
+    FrameCPP::Common::MD5Sum                       check_sum;
+    FrameCpp_hash_adapter< decltype( check_sum ) > hash_wrapper( check_sum );
 
     channel_t* cur = channels;
     channel_t* end = channels + num_channels;
-    for ( ; cur < end; ++cur )
-    {
-
-        check_sum.Update( &( cur->chNum ), sizeof( cur->chNum ) );
-        check_sum.Update( &( cur->seq_num ), sizeof( cur->seq_num ) );
-        size_t name_len = strnlen( cur->name, channel_t::channel_name_max_len );
-        check_sum.Update( cur->name, sizeof( name_len ) );
-        check_sum.Update( &( cur->sample_rate ), sizeof( cur->sample_rate ) );
-        check_sum.Update( &( cur->active ), sizeof( cur->active ) );
-        check_sum.Update( &( cur->trend ), sizeof( cur->trend ) );
-        check_sum.Update( &( cur->group_num ), sizeof( cur->group_num ) );
-        check_sum.Update( &( cur->bps ), sizeof( cur->bps ) );
-        check_sum.Update( &( cur->dcu_id ), sizeof( cur->dcu_id ) );
-        check_sum.Update( &( cur->data_type ), sizeof( cur->data_type ) );
-        check_sum.Update( &( cur->signal_gain ), sizeof( cur->signal_gain ) );
-        check_sum.Update( &( cur->signal_slope ), sizeof( cur->signal_slope ) );
-        check_sum.Update( &( cur->signal_offset ),
-                          sizeof( cur->signal_offset ) );
-        size_t unit_len =
-            strnlen( cur->signal_units, channel_t::engr_unit_max_len );
-        check_sum.Update( cur->signal_units, sizeof( unit_len ) );
+
+    std::function< void( const channel_t& ) > hash_cb{
+        [&hash_wrapper]( const channel_t& channel ) {
+            hash_channel( hash_wrapper, channel );
+        }
+    };
+    auto use_broken = parameters( ).get< int >(
+                          "USE_BROKEN_CONFIGURATION_NUMBER_HASH", 0 ) == 1;
+    if ( use_broken )
+    {
+        hash_cb = [&hash_wrapper]( const channel_t& channel ) {
+            hash_channel_v0_broken( hash_wrapper, channel );
+        };
     }
+    std::for_each( cur, end, hash_cb );
+
     check_sum.Finalize( );
 
     std::ostringstream ss;

src/daqd/tests/test_daqd_channel_hashes.cc

+//
+// Created by jonathan.hanks on 10/2/20.
+//
+
+#include "channel.hh"
+#include <cstring>
+#include <sstream>
+#include <string>
+
+#include "framecpp/Common/MD5SumFilter.hh"
+#include "catch.hpp"
+
+namespace
+{
+    channel_t
+    channel1( )
+    {
+        channel_t ch{};
+
+        ch.chNum = 1000;
+        ch.seq_num = 1;
+        ch.id = nullptr;
+        std::strcpy( ch.name, "CHANNEL1_THE_REAL_DEAL" );
+        ch.sample_rate = 16;
+        ch.active = 1;
+        ch.group_num = 1;
+        ch.bps = 16 * 4;
+        ch.offset = 0;
+        ch.bytes = 4;
+        ch.req_rate = 16;
+        ch.dcu_id = 1;
+        ch.ifoid = 0;
+        ch.tp_node = 1;
+        ch.rm_offset = 0;
+        ch.data_type = daq_data_t::_32bit_float;
+        ch.signal_gain = 1.0;
+        ch.signal_slope = 1.0;
+        ch.signal_offset = 0.0;
+        std::strcpy( ch.signal_units, "SOMETHING" );
+        return ch;
+    }
+
+    channel_t
+    channel2( )
+    {
+        channel_t ch{};
+
+        ch.chNum = 1000;
+        ch.seq_num = 1;
+        ch.id = nullptr;
+        std::strcpy( ch.name, "CHANNEL1_IMPOSTER" );
+        ch.sample_rate = 16;
+        ch.active = 1;
+        ch.group_num = 1;
+        ch.bps = 16 * 4;
+        ch.offset = 0;
+        ch.bytes = 4;
+        ch.req_rate = 16;
+        ch.dcu_id = 1;
+        ch.ifoid = 0;
+        ch.tp_node = 1;
+        ch.rm_offset = 0;
+        ch.data_type = daq_data_t::_32bit_float;
+        ch.signal_gain = 1.0;
+        ch.signal_slope = 1.0;
+        ch.signal_offset = 0.0;
+        std::strcpy( ch.signal_units, "SOMETHING_ELSE" );
+        return ch;
+    }
+
+    channel_t
+    channel3( )
+    {
+        channel_t ch{};
+
+        ch.chNum = 1001;
+        ch.seq_num = 1;
+        ch.id = nullptr;
+        std::strcpy( ch.name, "NOT_LIKE_CHANNEL1_IMPOSTER" );
+        ch.sample_rate = 16;
+        ch.active = 1;
+        ch.group_num = 1;
+        ch.bps = 16 * 4;
+        ch.offset = 0;
+        ch.bytes = 4;
+        ch.req_rate = 16;
+        ch.dcu_id = 1;
+        ch.ifoid = 0;
+        ch.tp_node = 1;
+        ch.rm_offset = 0;
+        ch.data_type = daq_data_t::_32bit_float;
+        ch.signal_gain = 1.0;
+        ch.signal_slope = 1.0;
+        ch.signal_offset = 0.0;
+        std::strcpy( ch.signal_units, "SOMETHING_ELSE_ENTIRELY" );
+        return ch;
+    }
+
+    std::string
+    test_hash_this_channel_v0( const channel_t& ch )
+    {
+        FrameCPP::Common::MD5Sum                       check_sum;
+        FrameCpp_hash_adapter< decltype( check_sum ) > hash_wrapper(
+            check_sum );
+
+        hash_channel_v0_broken( hash_wrapper, ch );
+
+        check_sum.Finalize( );
+        std::ostringstream os;
+        os << check_sum;
+        return os.str( );
+    }
+
+    std::string
+    test_hash_this_channel_v1( const channel_t& ch )
+    {
+        FrameCPP::Common::MD5Sum                       check_sum;
+        FrameCpp_hash_adapter< decltype( check_sum ) > hash_wrapper(
+            check_sum );
+
+        hash_channel( hash_wrapper, ch );
+
+        check_sum.Finalize( );
+        std::ostringstream os;
+        os << check_sum;
+        return os.str( );
+    }
+} // namespace
+
+TEST_CASE(
+    "hash_channel_v0_broken has easy collisions, reproduce buggy behavior" )
+{
+
+    auto hash1 = test_hash_this_channel_v0( channel1( ) );
+    auto hash2 = test_hash_this_channel_v0( channel2( ) );
+
+    REQUIRE( hash1 == hash2 );
+
+    REQUIRE( hash1 == "1353dc82bb61dddaf97ffdf2400206d0" );
+}
+
+TEST_CASE( "hash_channel_v0_broken doesn't collide on everything" )
+{
+    auto hash1 = test_hash_this_channel_v0( channel1( ) );
+    auto hash3 = test_hash_this_channel_v0( channel3( ) );
+
+    REQUIRE( hash1 != hash3 );
+
+    REQUIRE( hash3 == "c45a8cc2d02de142d87c89d260e7f6d5" );
+}
+
+TEST_CASE( "hash_channel does not have the same collision issue as "
+           "hash_channel_v0_broken" )
+{
+    auto hash1 = test_hash_this_channel_v1( channel1( ) );
+    auto hash2 = test_hash_this_channel_v1( channel2( ) );
+    auto hash3 = test_hash_this_channel_v1( channel3( ) );
+
+    REQUIRE( hash1 != hash2 );
+    REQUIRE( hash1 != hash3 );
+    REQUIRE( hash2 != hash3 );
+
+    REQUIRE( hash1 == "5bd1825ca5aaf1f1354ac3d39fe9a5e3" );
+    REQUIRE( hash2 == "7aa4d0506697a4d7ad7d5268a8a38389" );
+    REQUIRE( hash3 == "6b194dd0ec444ab919072d94ed95c130" );
+}