Skip to content
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

Move the element size check in EbmlCallbacks #273

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

robUx4
Copy link
Contributor

@robUx4 robUx4 commented Feb 25, 2024

Implements #272

And so we can check the validity of the size before an element is created.

@robUx4 robUx4 added api-break breaks the API (e.g. programs using it will have to adjust their source code) abi-break breaks the ABI (e.g. programs linked against the library have to be recompiled) labels Feb 25, 2024
@robUx4 robUx4 force-pushed the spec_length branch 2 times, most recently from 4dad849 to 1c12964 Compare February 25, 2024 15:55
@mbunkus
Copy link
Contributor

mbunkus commented Feb 25, 2024

Unfortunately these changes cause aborts due to failed assertions when reading data from damaged files:

[0 mosu@sweet-chili (main) ~/prog/video/mkvtoolnix/tests] mkvinfo data/mkv/sample-bug2989.mkv
+ EBML head
|+ EBML version: 1
|+ EBML read version: 1
|+ Maximum EBML ID length: 4
|+ Maximum EBML size length: 8
|+ Document type: matroska
|+ Document type version: 4
|+ (Unknown element: DummyElement; ID: 0x4220 size: 4)
+ Segment: size 129
mkvinfo: lib/libebml/src/EbmlElement.cpp:327: static EbmlElement *libebml::EbmlElement::FindNextElement(IOCallback &, const EbmlSemanticContext &, int &, std::uint64_t, bool, unsigned int): Assertion `Result->ElementSpec().IsSizeValid(SizeFound)' failed.
zsh: IOT instruction (core dumped)  mkvinfo data/mkv/sample-bug2989.mkv

This is the same old topic: reading invalid files must not abort the process (be strict in what you write/produce but lenient in what you read/accept).

@robUx4
Copy link
Contributor Author

robUx4 commented Feb 26, 2024

The assert is not there to break on bogus files, but to double check on the coherence of the code. The code that creates Result is probably correct. But it probably has an "infinite" size which is not handled in that test. I'll do a fix.

@robUx4
Copy link
Contributor Author

robUx4 commented Feb 26, 2024

And thanks for testing this (and the rest) by the way. I need to finish porting mkvalidator and mkclean as I would probably have caught it earlier.

@robUx4 robUx4 force-pushed the spec_length branch 2 times, most recently from 0a9e927 to 62962c0 Compare February 26, 2024 07:12
@robUx4
Copy link
Contributor Author

robUx4 commented Feb 26, 2024

@mbunkus can you check with this new version ? Now the size needs to check if this a finite or infinite size value. The code behavior should be the same as before. Finite size elements with a bogus size were not accepted before (created by then discarded).

If it still fails, can you share the file ?

@mbunkus
Copy link
Contributor

mbunkus commented Mar 2, 2024

A small ask for the future: when you publish PRs in libEBML for which a corresponding change is required in libMatroska, please post in the libEBML PR the number of the libMatroska PR & vice versa. Makes it much easier for me to go through all of those changes. Thanks!

@mbunkus
Copy link
Contributor

mbunkus commented Mar 2, 2024

After testing your updated changes, reading certain damaged files still aborts with failed assertions:

[0 mosu@sweet-chili (main) ~/prog/video/mkvtoolnix/tests] mkvinfo data/mkv/sample-bug2989.mkv
+ EBML head
|+ EBML version: 1
|+ EBML read version: 1
|+ Maximum EBML ID length: 4
|+ Maximum EBML size length: 8
|+ Document type: matroska
|+ Document type version: 4
|+ (Unknown element: DummyElement; ID: 0x4220 size: 4)
+ Segment: size 129
mkvinfo: lib/libebml/src/EbmlElement.cpp:323: static EbmlElement *libebml::EbmlElement::FindNextElement(IOCallback &, const EbmlSemanticContext &, int &, std::uint64_t, bool, unsigned int): Assertion `Result->ElementSpec().IsSizeValid(SizeFound, SizeFound == SizeUnknown)' failed.

This is the file from above, but I have others.

@mbunkus
Copy link
Contributor

mbunkus commented Mar 3, 2024

Can you please re-base on current master? I'll give it another try afterwards. Thanks.

@robUx4
Copy link
Contributor Author

robUx4 commented Mar 4, 2024

I rebased the code but did not do any further investigation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abi-break breaks the ABI (e.g. programs linked against the library have to be recompiled) api-break breaks the API (e.g. programs using it will have to adjust their source code)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants