-
Notifications
You must be signed in to change notification settings - Fork 149
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
isomp4, aac: validation improvements, fixes #301 #331
base: dev-0.6
Are you sure you want to change the base?
Conversation
Was:
Became:
|
941897b
to
5d913fd
Compare
added a new commit to fix aac panics. refactored aac aproximate_frame_count() method. It had some issues which causes panic. Should fix cases from https://github.com/qarmin/Automated-Fuzzer/actions/runs/12516290494 (REPORTS___SYMPHONIA*)
I haven't tested on VBR aac to observe any improvements, It was mention that aproximate method is not perfect there @dedobbin please review. |
f970b72
to
2c2a8e4
Compare
fixed elst atom entry count sizes |
Added commit: core: advance reader's position if pattern cannot be found This addresses out-of-memory errors mentioned in the following comment: Issue #301 Comment. Issue description:
This commit ensures that the reader's position is always advanced, resolving the issue. |
Added commit: convert FourCc assertions to decode_error This addresses some panic errors mentioned in the following comment: #301 (comment). |
Added commit: mkv: report decode error on invalid vint width This addresses some panic errors mentioned in the following comment: #301 (comment). |
Happy new year, @sscobici! I'll be back to working on Symphonia in a few days, however, I wanted to drop in here to give you a heads up since you're touching a lot of files. I have a WIP change that rewrites MKV's EBML iterator entirely. The previous code was not fully compliant with the spec, but also had many fundamental issues. So please avoid modifying the MKV reader as any changes there will become a conflict. While working on those changes, I also realized that MP4, RIFF, and MKV are all roughly the same (i.e., nested chunk based structure) and thus all had roughly the same problems (e.g., reading out of bounds, etc.). Therefore, I'd like to apply a similar approach as the new EBML iterator to MP4's Atom iterator, and RIFF's Chunk iterator. In this PR I noticed that you've added size checks to some atom. I'm not very fond of this solution because it's not a very scalable solution, and is difficult for parent atoms. Also, it involves a lot of "magic numbers". I think the new EBML iterator provides a good solution to this problem that's more general. So, I'd also recommend avoiding making further changes like this as well. The new EBML iterator requires some additional polishing before I'm ready to merge it, but I just wanted to give you this heads up. |
ee647d2
to
d3cd3fd
Compare
Added commit: aac: add validation for Ics.info.max_sfb, fixes out of range panic during decoding. |
Added More Validations for isomp4 to prevent some panics, Fixes #301
Note: This update does not resolve all potential panic scenarios or address issues related to high memory consumption.
Requires a full regression run on as many files as possible to revalidate the constants put for validations.