libacm crash in UnpackLinear

[MaddTheSane]

When running on an Apple Silicon Mac, I've run into an issue with libacm, specifically the InternalAudioDecoder::UnpackLinear function.

https://github.com/kevinbentley/Descent3/blob/1edaa3b628b6945f93f7fde453967e244fabb45b/libacm/libacm.cpp#L574-L589

It seems like when id is 9 (and thus middleIndex gets set to 256) and offset gets set to 255, it results in trying to get a negative address at decoder.m_pMidBuffer, which causes a crash.

[JeodC]

Maybe do something like this?

     if (offset < middleIndex) {
       //Set pBlock[pos] to a default value, return false, or log an error
     }

This would go above

    const uint32 pos = (i << level) + col; 
    pBlock[pos] = decoder.m_pMidBuffer[offset - middleIndex]; 

[kevinbentley]

I've been debugging this same issue under Linux. I've been playing whack a mole trying to catch all the negative values, but we need to get to the root of the problem. It might be worth looking at switching to this library, which must be pretty good because it's part of ffmpeg now. https://github.com/markokr/libacm

[midnite8177]

I bet GetBitsEOF has the bug Probably worth trying out that library

[midnite8177]

Marko Kreen wrote the original libacm, so this must be related

[bryanperris]

What about creating unit tests?

[devint1]

The issue seems to be that the C++ compiler treats offset - middleIndex as uint32 instead of sint32. Trying to force it to a non-negative value produces audio distortion, so I suppose a negative index is expected, althought I don't know enough about the ACM codec to know if this is technically correct.

This seems to fix the issue for now:

// pBlock[pos] = decoder.m_pMidBuffer[offset - middleIndex]; 
pBlock[pos] = decoder.m_pMidBuffer[((sint32) offset) - middleIndex];

[DanielGibson]

The libacm code in this repo is very different than the upstream code (which is plain C). Might make sense to import and use the current libacm code https://github.com/markokr/libacm/? Probably has this and other bugs fixed

UPDATE: Working on that (my plan is to rewrite InternalAudioDecoder in libacm.cpp to be a minimal wrapper that uses the libacm C interface, so libacm can be easily updated in the future). WIP branch: https://github.com/DanielGibson/Descent3/tree/use-libacm (only preparations so far, I hope to do the interesting part tomorrow)

[Jayman2000]

You can work around this problem by running Descent 3 with the --nosound option.

[JeodC]

UPDATE: Working on that (my plan is to rewrite InternalAudioDecoder in libacm.cpp to be a minimal wrapper that uses the libacm C interface, so libacm can be easily updated in the future). WIP branch: https://github.com/DanielGibson/Descent3/tree/use-libacm (only preparations so far, I hope to do the interesting part tomorrow)

Appreciated, thank you!

[DanielGibson]

Done, see #135