The +1 is OK, since the resize() calls also have a +1:
out.stats_by_depth.resize((STATS_MAX_DEPTH / STATS_DEPTH_BUCKET) + 1);
out.stats_by_temp.resize((STATS_MAX_TEMP / STATS_TEMP_BUCKET) + 1);
but the std::clamp() are not OK, because the interval is inclusive. They should
read
d_idx = std::clamp(d_idx, 0, STATS_MAX_DEPTH / STATS_DEPTH_BUCKET - 1);
t_idx = std::clamp(t_idx, 0, STATS_MAX_TEMP / STATS_TEMP_BUCKET - 1);
Berthold
On Samstag, 11. Jänner 2025 00:53:06 WIB Dirk Hohndel via subsurface wrote:
> Hmm. That's weird code:
>
> int d_idx = dp->maxdepth.mm / (STATS_DEPTH_BUCKET * 1000);
> d_idx = std::clamp(d_idx, 0, STATS_MAX_DEPTH / STATS_DEPTH_BUCKET);
> process_dive(*dp, out.stats_by_depth[d_idx + 1]);
>
> So we make sure that d_idx is an allowable index value, and then add 1 to
> it... I could see how that might access beyond the end of the array.
>
> /D
>
> > On Jan 10, 2025, at 09:46, Christof Arnosti via subsurface
> > <[email protected]> wrote:
> >
> > Check the passed stats_t reference, this is the structure that is read
> > there. I guess the pointer passed there is outside of the
> > out.stat_by_depth array. The field "total_average_depth_time" is quite at
> > the beginning of this structure, thus the 8 byte offset makes sense.
> >
> > Best
> > Christof
> >
> > On 10.01.25 18:33, Dirk Hohndel via subsurface wrote:
> >> I ended up using the ASAN build we already have configured in cmake:
> >>
> >> =================================================================
> >> ==960805==ERROR: AddressSanitizer: heap-buffer-overflow on address
> >> 0x520000010e18 at pc 0x56f170bc8a05 bp 0x7ffcd0552140 sp 0x7ffcd0552130
> >> READ of size 4 at 0x520000010e18 thread T0
> >>
> >> #0 0x56f170bc8a04 in process_dive
> >> /home/hohndel/src/subsurface/core/statistics.cpp:50 #1
> >> 0x56f170bca05e in calculate_stats_summary(bool)
> >> /home/hohndel/src/subsurface/core/statistics.cpp:162 #2
> >> 0x56f170a2dbef in exportHTMLstatistics
> >> /home/hohndel/src/subsurface/core/divelogexportlogic.cpp:81 #3
> >> 0x56f170a307fd in exportHtmlInitLogic(QString const&,
> >> htmlExportSetting&)
> >> /home/hohndel/src/subsurface/core/divelogexportlogic.cpp:142 #4
> >> 0x56f1709f89a3 in main
> >> /home/hohndel/src/subsurface/export-html.cpp:67 #5 0x7522d4c2a1c9 in
> >> __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58 #6
> >> 0x7522d4c2a28a in __libc_start_main_impl ../csu/libc-start.c:360 #7
> >> 0x56f1709f7aa4 in _start
> >> (/srv/hohndel/src/subsurface/build/export-html+0x236aa4) (BuildId:
> >> 3185437f967b115b8c1c9b6c8b5b8fe3d46e49bb)>>
> >> 0x520000010e18 is located 8 bytes after 3472-byte region
> >> [0x520000010080,0x520000010e10)>>
> >> allocated by thread T0 here:
> >> #0 0x7522d88fe548 in operator new(unsigned long)
> >> ../../../../src/libsanitizer/asan/asan_new_delete.cpp:95 #1
> >> 0x56f170bd0eef in std::__new_allocator<stats_t>::allocate(unsigned
> >> long, void const*) /usr/include/c++/13/bits/new_allocator.h:151 #2
> >> 0x56f170bd051e in std::allocator_traits<std::allocator<stats_t>
> >> >::allocate(std::allocator<stats_t>&, unsigned long)
> >> /usr/include/c++/13/bits/alloc_traits.h:482 #3 0x56f170bd051e in
> >> std::_Vector_base<stats_t, std::allocator<stats_t>
> >> >::_M_allocate(unsigned long)
> >> /usr/include/c++/13/bits/stl_vector.h:381 #4 0x56f170bcf11d in
> >> std::vector<stats_t, std::allocator<stats_t>
> >> >::_M_default_append(unsigned long)
> >> /usr/include/c++/13/bits/vector.tcc:663 #5 0x56f170bcd6ba in
> >> std::vector<stats_t, std::allocator<stats_t> >::resize(unsigned
> >> long) /usr/include/c++/13/bits/stl_vector.h:1016 #6 0x56f170bc96bb
> >> in calculate_stats_summary(bool)
> >> /home/hohndel/src/subsurface/core/statistics.cpp:119 #7
> >> 0x56f170a2dbef in exportHTMLstatistics
> >> /home/hohndel/src/subsurface/core/divelogexportlogic.cpp:81 #8
> >> 0x56f170a307fd in exportHtmlInitLogic(QString const&,
> >> htmlExportSetting&)
> >> /home/hohndel/src/subsurface/core/divelogexportlogic.cpp:142 #9
> >> 0x56f1709f89a3 in main
> >> /home/hohndel/src/subsurface/export-html.cpp:67 #10 0x7522d4c2a1c9
> >> in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
> >> #11 0x7522d4c2a28a in __libc_start_main_impl ../csu/libc-start.c:360
> >> #12 0x56f1709f7aa4 in _start
> >> (/srv/hohndel/src/subsurface/build/export-html+0x236aa4) (BuildId:
> >> 3185437f967b115b8c1c9b6c8b5b8fe3d46e49bb)>>
> >> SUMMARY: AddressSanitizer: heap-buffer-overflow
> >> /home/hohndel/src/subsurface/core/statistics.cpp:50 in process_dive
> >>
> >> So clearly we are addressing data outside the structure. I haven't
> >> figured out how that could happen on line 50, though...
> >>
> >> /D
> >>
> >>> On Jan 10, 2025, at 08:42, Berthold Stoeger <[email protected]>
> >>> <mailto:[email protected]> wrote:
> >>>
> >>> Hi Dirk,
> >>>
> >>> please run a debug version under valgrind and send me the result. A
> >>> minimal reproducing example would also be nice (repeatedly delete one
> >>> half of the dives and check if the issue is still there).
> >>>
> >>> N.B.: I'm in Indonesia until end of February and therefore will not be
> >>> very responsive. I'll try to check my e-mails once per day.
> >>>
> >>> Berthold
_______________________________________________
subsurface mailing list -- [email protected]
To unsubscribe send an email to [email protected]
