ldmx-sw icon indicating copy to clipboard operation
ldmx-sw copied to clipboard

Potential out-of-bounds memory in IdealClusterBuilder

Open tvami opened this issue 7 months ago • 7 comments

Describe the bug

In https://github.com/LDMX-Software/ldmx-sw/actions/runs/15058735763/job/42329736694 ASAN gives us

[ 94%] Building CXX object TrigScint/CMakeFiles/TrigScint.dir/src/TrigScint/QIEInputPulse.cxx.o
In file included from /usr/include/c++/13/algorithm:60,
                 from /home/runner/work/ldmx-sw/ldmx-sw/Trigger/Algo/include/Trigger/IdealClusterBuilder.h:4,
                 from /home/runner/work/ldmx-sw/ldmx-sw/Trigger/Algo/src/Trigger/IdealClusterBuilder.cxx:1:
In static member function 'static constexpr _Up* std::__copy_move<_IsMove, true, std::random_access_iterator_tag>::__copy_m(_Tp*, _Tp*, _Up*) [with _Tp = const trigger::Hit; _Up = trigger::Hit; bool _IsMove = false]',
    inlined from 'constexpr _OI std::__copy_move_a2(_II, _II, _OI) [with bool _IsMove = false; _II = const trigger::Hit*; _OI = trigger::Hit*]' at /usr/include/c++/13/bits/stl_algobase.h:506:30,
    inlined from 'constexpr _OI std::__copy_move_a1(_II, _II, _OI) [with bool _IsMove = false; _II = const trigger::Hit*; _OI = trigger::Hit*]' at /usr/include/c++/13/bits/stl_algobase.h:533:42,
    inlined from 'constexpr _OI std::__copy_move_a(_II, _II, _OI) [with bool _IsMove = false; _II = const trigger::Hit*; _OI = trigger::Hit*]' at /usr/include/c++/13/bits/stl_algobase.h:540:31,
    inlined from 'constexpr _OI std::copy(_II, _II, _OI) [with _II = const trigger::Hit*; _OI = trigger::Hit*]' at /usr/include/c++/13/bits/stl_algobase.h:633:7,
    inlined from 'constexpr void std::vector<_Tp, _Alloc>::_M_assign_aux(_ForwardIterator, _ForwardIterator, std::forward_iterator_tag) [with _ForwardIterator = const trigger::Hit*; _Tp = trigger::Hit; _Alloc = std::allocator<trigger::Hit>]' at /usr/include/c++/13/bits/vector.tcc:341:15,
    inlined from 'constexpr void std::vector<_Tp, _Alloc>::_M_assign_aux(_ForwardIterator, _ForwardIterator, std::forward_iterator_tag) [with _ForwardIterator = const trigger::Hit*; _Tp = trigger::Hit; _Alloc = std::allocator<trigger::Hit>]' at /usr/include/c++/13/bits/vector.tcc:315:7,
    inlined from 'constexpr std::vector<_Tp, _Alloc>& std::vector<_Tp, _Alloc>::operator=(std::initializer_list<_Tp>) [with _Tp = trigger::Hit; _Alloc = std::allocator<trigger::Hit>]' at /usr/include/c++/13/bits/stl_vector.h:790:21,
    inlined from 'void trigger::IdealClusterBuilder::Build2dClusters()' at /home/runner/work/ldmx-sw/ldmx-sw/Trigger/Algo/src/Trigger/IdealClusterBuilder.cxx:215:27:
/usr/include/c++/13/bits/stl_algobase.h:437:30: warning: 'void* __builtin_memmove(void*, const void*, long unsigned int)' reading between 45 and 9223372036854775807 bytes from a region of size 44 [-Wstringop-overread]
  437 |             __builtin_memmove(__result, __first, sizeof(_Tp) * _Num);
      |             ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/runner/work/ldmx-sw/ldmx-sw/Trigger/Algo/src/Trigger/IdealClusterBuilder.cxx: In member function 'void trigger::IdealClusterBuilder::Build2dClusters()':
/home/runner/work/ldmx-sw/ldmx-sw/Trigger/Algo/src/Trigger/IdealClusterBuilder.cxx:215:27: note: source object '<anonymous>' of size 44
  215 |       layer_hits[l] = {hit};
      |  

Ways to reproduce should be

just configure-asan-ubsan 
just build
just fire .github/validation_samples/ecal_pn/config.py 

Desired behavior

Clean compilation

tvami avatar May 16 '25 02:05 tvami

Could be false-postive? @EinarElen wdyt?

tvami avatar May 16 '25 02:05 tvami

False positives from asan are really really uncommon so I'd be surprised

EinarElen avatar May 16 '25 05:05 EinarElen

Oh wait this is a compile time thing, not ASAN...

(Also accidentally closed and reopened, my bad...)

EinarElen avatar May 16 '25 06:05 EinarElen

Yes it is compiler, but I noticed it in the ASAN build

tvami avatar May 16 '25 15:05 tvami

hi @therwig did you write this class? I also tried to add some of it in the logging system and get rid of the Printouts in https://github.com/LDMX-Software/ldmx-sw/pull/1800 but it lead me down on a rabbit hole and then I just gave up.

Anyway, I think this class needs a bit of a maintenance, can I add you as an assignee for it?

tvami avatar Aug 04 '25 18:08 tvami

Yes, I wrote it a while ago and can take a look if its generating some errors.

therwig avatar Aug 04 '25 18:08 therwig

I think if you do

just configure-asan-ubsan 
just build
just fire .github/validation_samples/ecal_pn/config.py 

the "potential out-of-bounds" should come up! Altho I did test that recently.

What I tried to do recently is to move the cout into ldmx_log like https://github.com/LDMX-Software/ldmx-sw/blob/7c8566a1d3ec34b9e39da83216beb832786d3a93/Trigger/Algo/src/Trigger/IdealClusterBuilder.cxx#L96 etc and that didnt work out

I also tried to migrate away from the Print method like in https://github.com/LDMX-Software/ldmx-sw/blob/7c8566a1d3ec34b9e39da83216beb832786d3a93/Trigger/Algo/src/Trigger/IdealClusterBuilder.cxx#L97 and I gave up on that too.

tvami avatar Aug 04 '25 18:08 tvami