AlexandreRouma / SDRPlusPlus

Cross-Platform SDR Software
GNU General Public License v3.0
4.18k stars 579 forks source link

Crash in 'kg_sstv_decoder.dll' #742

Closed gvanem closed 2 years ago

gvanem commented 2 years ago

When playing with the kg_sstv_decoder, I hit this crash. From WinDbg:

Access violation - code c0000005 (!!! second chance !!!)
eax=5d21a040 ebx=ffffffee ecx=00000014 edx=00000000 esi=00000003 edi=0f793654
eip=6298d7f0 esp=65cef90c ebp=65cef95c iopl=0         nv up ei pl nz na po nc
cs=0023  ss=002b  ds=002b  es=002b  fs=0053  gs=002b             efl=00010202
kg_sstv_decoder!kgsstv::Deframer::run+0x40:
6298d7f0 c5fa100498      vmovss  xmm0,dword ptr [eax+ebx*4] ds:002b:5d219ff8=????????

Call stack:

kg_sstv_decoder!kgsstv::Deframer::run(void)+0x40
kg_sstv_decoder!dsp::generic_block<kgsstv::Deframer>::workerLoop(void)+0xa
kg_sstv_decoder!std::invoke(void)+0xa
kg_sstv_decoder!std::thread::_Invoke<std::tuple<void (void * _RawVals = 0x0ed4ad80)+0x23
ucrtbase!thread_start<unsigned int +0x3f
...

Since the vmoss instruction is not reading from a 16-byte aligned address (it is ds:002b:5d219ff8). I built everything with MSVC-2019 and option -arch:AVX.

Seems that _in->readBuf[i] must be properly aligned here:

     for (int i = 0; i < count; i++) {
                if (syncing) {
                    // If sync broken, reset sync
                    if ((_in->readBuf[i] > 0.0f) && !KGSSTV_SYNC_WORD[match]) {  // << here

But I fail to understand how/where that alignment should be done.

gvanem commented 2 years ago

It's not an alignment issue after all. But that i becomes negative:

            for (int i = 0; i < count; i++) {
                if (syncing) {
                    // If sync broken, reset sync
                    if ((_in->readBuf[i] > 0.0f) && !KGSSTV_SYNC_WORD[match]) {
                        if (++err > 4) {
                            i -= match - 1;  // here
                            match = 0;
                            err = 0;
                            continue;
                        }
                    }

causing an illegal read of address _in->readBuf[i].

A fix for me was to patch:

--- a/decoder_modules/kg_sstv_decoder/src/kg_sstv_dsp.h 2022-04-02 13:22:49
+++ b/decoder_modules/kg_sstv_decoder/src/kg_sstv_dsp.h 2022-04-26 12:20:51
@@ -142,7 +142,7 @@
             int count = _in->read();
             if (count < 0) { return -1; }

-            for (int i = 0; i < count; i++) {
+            for (int i = 0; i > 0 && i < count; i++) {
                 if (syncing) {
                     // If sync broken, reset sync
                     if ((_in->readBuf[i] > 0.0f) && !KGSSTV_SYNC_WORD[match]) {

This works (i.e. doesn't crash and I see the decoder window). But I have no access to any SSTV data to test with.

PS. this module seems to be a copy & paste of M17; class M17DecoderModule.

AlexandreRouma commented 2 years ago

That decoder isn't listed in the modules for a reason. It's an unfinished prototype that has no functionality whatsoever.

Never expect a module not listed as "Working/Beta" (or not listed at all) to work.

gvanem commented 2 years ago

Then what is the OPT_BUILD_KG_SSTV_DECODER option for? But I did not use CMake to built sdrpp.exe.

AlexandreRouma commented 2 years ago

For me to use when I'm working on the prototype. It's off by default.