This solves maybe the issue which appeared in BUG: 389540. At least, I don't get any invalid access warnings from asan anymore. However, the logic of the search is somewhat complex, so I'm not 100% sure that the correct branch is taken with the new access checks. Someone more familiar with the code might want to take a look at it.

I should add that KPatience relies on the Wp pointing one field before an array (which happens for instance Solver::unpack_position when strecpy returns 0, as the pile is empty). This apparently works with most compilers, but technically it's UB according to §5.7, 4 of the C++ standard (N4296). However, fixing this would require a large amount of refactoring.

In D10889#215015, @fabiank wrote:

This solves maybe the issue which appeared in BUG: 389540. At least, I don't get any invalid access warnings from asan anymore. However, the logic of the search is somewhat complex, so I'm not 100% sure that the correct branch is taken with the new access checks. Someone more familiar with the code might want to take a look at it.

I'm afraid we don't really have much people familiar with the code around anymore. I'll try to give it a look at some point. You were able to reproduce the crash/asan warning each time?

I always got the an asan abort due to the missing checks (the solver would always execute the code and dereference non-allocated memory ). Now, without asan and without the patches, I didn't get any crashes in a debug build, and occasionally crashes with RelWithDeb. I didn't get any crashes with RelWithDeb and my patches, though of course there could be other crab sources.

As I commented before, there's still some UB in the code,and unfortunately that's not easy to solve.

it's ok.

BTW are you sure this fixes bug 389540 ? The backtrace in there seems to indicate a crash in the patsolve destructor.

Also why playing the spider solitaire (1 suit) i've got it two crash two times with

ASAN:DEADLYSIGNAL

20238==ERROR: AddressSanitizer: SEGV on unknown address 0x00e19fff8002 (pc 0x5613c126fc8e bp 0x7f1b96d1a9d0 sp 0x7f1b96d1a9b0 T28)

20238==The signal is caused by a READ memory access.

#0 0x5613c126fc8d in MemoryManager::new_from_block(unsigned long) /home/tsdgeos/devel/kde/kpat/patsolve/memory.cpp:164
#1 0x5613c1272a0f in Solver::pack_position() /home/tsdgeos/devel/kde/kpat/patsolve/patsolve.cpp:372
#2 0x5613c127662b in Solver::insert(unsigned int*, int, TREE**) /home/tsdgeos/devel/kde/kpat/patsolve/patsolve.cpp:1045
#3 0x5613c1276857 in Solver::new_position(POSITION*, MOVE*) /home/tsdgeos/devel/kde/kpat/patsolve/patsolve.cpp:1076
#4 0x5613c1274a02 in Solver::solve(POSITION*) /home/tsdgeos/devel/kde/kpat/patsolve/patsolve.cpp:741
#5 0x5613c12742ff in Solver::doit() /home/tsdgeos/devel/kde/kpat/patsolve/patsolve.cpp:666
#6 0x5613c1275e53 in Solver::patsolve(int, bool) /home/tsdgeos/devel/kde/kpat/patsolve/patsolve.cpp:960
#7 0x5613c11f6c8c in SolverThread::run() (/home/tsdgeos/devel/kde/install/bin/kpat+0x8dc8c)
#8 0x7f1bb92d5b4c  (/usr/lib/libQt5Core.so.5+0xafb4c)
#9 0x7f1bb7f6208b in start_thread (/usr/lib/../lib/libpthread.so.0+0x708b)
#10 0x7f1bb867ae7e in __GI___clone (/usr/lib/libc.so.6+0xf5e7e)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /home/tsdgeos/devel/kde/kpat/patsolve/memory.cpp:164 in MemoryManager::new_from_block(unsigned long)
Thread T28 (SolverThread) created by T0 here:

#0 0x7f1bbf60c511 in __interceptor_pthread_create /build/gcc/src/gcc/libsanitizer/asan/asan_interceptors.cc:243
#1 0x7f1bb92d4ffa in QThread::start(QThread::Priority) (/usr/lib/libQt5Core.so.5+0xaeffa)

maybe you can also try to give it a look?

No, I'm not sure, it was probably more wishful thinking. Though those illegal reads can cause all kinds of memory corruption. Did your crash occur in a Debug build or in a RelWithDeb build? Just asking as the first makes it more likely to be an easy to find logic error, while the latter can be the compiler making use of UB at any place.

Debug

So the last change fixes that mp gets incremented and we happily write into the storage space of the MemoryManager which leads to all kinds of crashes. Now, I know that goto is normally not something you put into modern C++, but it seemed to be the nicest way out of the nested loop while keeping the check local to the increment.

Additionally, considering that those writes corrupt the object, this might explain the crash in the destructor in the bug report.

Friendly ping