Open tmiw opened 8 months ago
Thanks @tmiw. If you think you have a quick fix and don't mind working on it pls feel free to go ahead and raise a PR.
I created https://github.com/drowe67/codec2/pull/45 with what I was able to get to tonight. There's still something like 14 ctest failures with libasan on, but I suspect at least a couple are due to configuration issues on my end.
@tmiw - did any of the issues detected by AddressSanitizer cause any actual, real world issues, when running say freedv-gui? The changes in https://github.com/drowe67/codec2/pull/45 are minor tweaks mainly in test code for soon to be deprecated modes. All this code was running fine before this tool was applied.
We already have a suite of valgrind
tests that I guess traps most memory issues.
@tmiw - did any of the issues detected by AddressSanitizer cause any actual, real world issues, when running say freedv-gui? The changes in #45 are minor tweaks mainly in test code for soon to be deprecated modes. All this code was running fine before this tool was applied.
We already have a suite of
valgrind
tests that I guess traps most memory issues.
This originally kicked off because @barjac couldn't build with AddressSanitizer while attempting to debug a freedv-gui issue. Fixing that of course ended up kicking off additional issues (for example, the behavior apparently varying wildly depending on what locale freedv-gui is set to). Also, I'm fairly sure he was able to duplicate the crashes in freedv-gui without AddressSanitizer enabled at one point, too.
For reference, the most recent crashes seemed to be happening while trying to decode 800XA:
==261635==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x602000185637 at pc 0x7f503ae47971 bp 0x7f501a342f50 sp 0x7f501a342700
WRITE of size 8 at 0x602000185637 thread T23
#0 0x7f503ae47970 in __interceptor_memset (/lib64/libasan.so.8+0x47970)
#1 0x7f503acbc176 in fvhff_extract_frame_voice /home/baz/BLD/BLD_MgaX_FREEDV/freedv-git/SOURCES/freedv-1.9.9-202403221300-3e864/codec2/src/freedv_vhf_framing.c:556
#2 0x7f503acbef19 in fvhff_extract_frame /home/baz/BLD/BLD_MgaX_FREEDV/freedv-git/SOURCES/freedv-1.9.9-202403221300-3e864/codec2/src/freedv_vhf_framing.c:738
#3 0x7f503acbef19 in fvhff_deframe_bits /home/baz/BLD/BLD_MgaX_FREEDV/freedv-git/SOURCES/freedv-1.9.9-202403221300-3e864/codec2/src/freedv_vhf_framing.c:855
#4 0x7f503acbaf18 in freedv_comprx_fsk /home/baz/BLD/BLD_MgaX_FREEDV/freedv-git/SOURCES/freedv-1.9.9-202403221300-3e864/codec2/src/freedv_fsk.c:671
#5 0x7f503aca9d8a in freedv_comprx /home/baz/BLD/BLD_MgaX_FREEDV/freedv-git/SOURCES/freedv-1.9.9-202403221300-3e864/codec2/src/freedv_api.c:806
#6 0x4a45f8 in FreeDVReceiveStep::execute(std::shared_ptr<short>, int, int*) /home/baz/BLD/BLD_MgaX_FREEDV/freedv-git/SOURCES/freedv-1.9.9-202403221300-3e864/src/pipeline/FreeDVReceiveStep.cpp:100
(and earlier, were happening when decoding 2020 too)
There are also security implications, too. While admittedly unlikely, I imagine someone could come up with an OTA signal (or even just a WAV file that they convinced someone to play back in freedv-gui) that triggered one of the buffer overflows fixed by this PR and caused them to gain control of the user's system or something.
Anyway, that's a long way of saying that these modes should be fixed as long as the modes are still being used by something (e.g. freedv-gui). :)
@tmiw - I'm not convinced this is an issue we should be addressing at this time.
We made a decision at PLT that non-critical bugs - especially in deprecated modes - would not be addressed at this time. Fixing ctests in deprecated code (that pass just fine) is not where we should working right now.
As codec 2 lead I'm making the call to pause any further libcodec2 work in this area - if you'd like to discuss further please raise at PLT.
@barjac reported an AddressSanitizer error during freedv-gui build:
Additionally, it appears a large number of ctests fail with AddressSanitizer enabled. Building with the following on an aarch64 Ubuntu VM:
(More tests were failing without disabling the ODR detection. I disabled above since it seemed like a false alarm.)
Example of one of the ctest failures:
A quick look seems to indicate that it's due to the following definition for
my_cb_state
(in bothfreedv_rx
andfreedv_tx
):This definition should be moved to the top of
main()
to eliminate this error.