SoundSourceM4A: Reconfigure decoder after channel config errors #2850
Conversation
|
Any reasons why we didn't drop FAAD when FFmpeg support was added? |
|
Offsets for cue points might differ slightly. On the other hand it would allow us to drop libmp4v2 which also seems to be discontinued. |
|
The offsets would be the same across all OSes, right? If we find a pattern we can correct them with a DB migration. We should switch eventually IMHO. |
Implicit channel mapping reconfiguration is explicitely forbidden by ISO/IEC 13818-7:2006 (8.5.3.3). Decoders should be able to detect such files and reject them. FAAD2 does not perform any kind of checks regarding this. This leads to security vulnerabilities when processing crafted AAC files performing such reconfigurations. Add checks to decode_sce_lfe and decode_cpe to make sure such inconsistencies are detected as early as possible. These checks first read hDecoder->frame: if this is not the first frame then we make sure that the syntax element at the same position in the previous frame also had element_id id_syn_ele. If not, return 21 as this is a fatal file structure issue. This patch addresses CVE-2018-20362 (fixes #26) and possibly other related issues.
There was a problem hiding this comment.
Thank you for the fix and all the useful refactoring.
I am unsure how we should continue here in the long run. Since libmp4v2 was removed from debian.
The faad guys have fixed it by adding their own mp4 stream extraction functions.
During review I have done some fixes
https://github.com/uklotzde/mixxx/pull/15
src/sources/soundsourcem4a.cpp
Outdated
| writableSampleFrames.writableData(), | ||
| std::min(writableSampleFrames.writableLength(), numberOfSamples))); | ||
| DEBUG_ASSERT(isValidFrameIndex(m_curFrameIndex)); | ||
| if (!retryAfterReopeningDecoder) { |
There was a problem hiding this comment.
I think this condition is always true.
There was a problem hiding this comment.
No it is not. After exiting the inner loop we will end up here. But now we can continue, see my changes.
| m_curSampleBlockId = MP4_INVALID_SAMPLE_ID; | ||
| m_inputBufferLength = 0; | ||
| if (!reopenDecoder()) { | ||
| return {}; |
There was a problem hiding this comment.
Why returning a empty initializer list, that compiles to a default constructed void();
That is confusing. Please use just return;
This has the advantage that the compiler will complain when we change the function returning something sensible.
Unlike changing the signature from int to double or such we need to reconsider every return path when changing from void.
Please also fix the other occurrences.
There was a problem hiding this comment.
Incorrect, please expand the diff.
It returns an empty, default constructed ReadableSampleFrames without redundantly repeating the type name.
Faad2 + CMake
|
Ping. This is needed for fixing .m4a playback on Fedora 32, replacing 2.2 with 2.3 beta. Otherwise I have to switch from FAAD2 to FFmpeg permanently. |
| // At this point we have failed to decode the current sample | ||
| // block twice and need to discard it. The content of the | ||
| // sample block is unknown and we simply continue with the | ||
| // next block. This is just a workaround! The reason why FAAD2 |
There was a problem hiding this comment.
by using silence instead ..
or
by skipping the block
I am afraid that the later will produce a cue point offset if the track was originally decoded with a working FAAD2 decoder or if it was decoded with a broken decoder the offset will appear after the decoder is fixed.
There was a problem hiding this comment.
I am not writing an AAC decoder for the unknown frames rejected blocks. No chance. In this case I will close this PR.
There was a problem hiding this comment.
We don't even know if and how many samples those blocks contain. Sorry, but this is not doable.
|
No, need to fix that. I was just curious to know what this patch means. |
|
sigh |
A fix for the evil FAAD2 2.9.x that required a huge amount of work and testing. Continued and finished after recent discussions in #2738.
I still don't understand why we need all those complex workarounds to decode regular M4A files. VLC is also using lots of custom patches to fix issues.
If more issues arise I recommend to drop FAAD2 entirely. It's not worth to waste so much time on this.