Don't attempt to write deprecated elements #209
Conversation
If we want to never allow that (as today) we can do the same check on every call of the write filter which will give the same result. |
ebml/EbmlElement.h
Outdated
| return true; | ||
| // write all elements except deprecated ones | ||
| static bool WriteAll(const EbmlElement & elt) { | ||
| return elt.ElementSpec().GetVersions().minver != EbmlDocVersion::ANY_VERSION; |
There was a problem hiding this comment.
I don't understand this logic. How is "element is deprecated" the same as "element has a minimum version set"? Don't have elements such as LanguageIETF a minimum version 'cause they were introduced in much later revisions of Matroska?
There was a problem hiding this comment.
"deprecated" elements (that have minver and maxver set to 0) are marked with a minver of ANY_VERSION:
// the minimum DocType version this element is allowed in
// ANY_VERSION if the element is never supported
const num_version minver;
Maybe we can add a helper method in the EbmlDocVersion class that would hide how it's handeld internally.
There was a problem hiding this comment.
Maybe we can add a helper method in the
EbmlDocVersionclass that would hide how it's handeld internally.
Yes, please. That's what I asked for in #207 as well after thinking about it for a bit.
They should not be readable by anyone.
robUx4
force-pushed
the
min_deprecated
branch
from
2ac8edc
to
efd91a8
Compare
There was a problem hiding this comment.
All of my test cases segfault here when compiled with optimizations, but at least some work fine when compiled without optimizations. This indicates that there's likely some kind of use of undefined behavior.
Running valgrind on the unoptimized binaries doesn't show anything, wile it does show invalid memory access (including reading from 0x0) in the optimized binaries.
Here's an example:
[0 mosu@sweet-chili ~/prog/video/data] vg mkvmerge -o v.mkv v.avi
==2299633== Memcheck, a memory error detector
==2299633== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al.
==2299633== Using Valgrind-3.22.0 and LibVEX; rerun with -h for copyright info
==2299633== Command: mkvmerge -o v.mkv v.avi
==2299633==
mkvmerge v82.0.24 ('I'm The President') 64-bit
==2299633== Conditional jump or move depends on uninitialised value(s)
==2299633== at 0x55FCB8F: ???
==2299633== by 0xBC0A40F: ???
==2299633==
'v.avi': Using the demultiplexer for the format 'AVI'.
'v.avi' track 0: Using the output module for the format 'AVC/H.264 (unframed)'.
'v.avi' track 1: Using the output module for the format 'AAC'.
The file 'v.mkv' has been opened for writing.
==2299633== Invalid read of size 4
==2299633== at 0x3CF1EB: IsAlwaysDeprecated (EbmlElement.h:283)
==2299633== by 0x3CF1EB: libebml::EbmlElement::WriteAll(libebml::EbmlElement const&) (EbmlElement.h:462)
==2299633== by 0x71CB34: libebml::EbmlElement::Render(libebml::IOCallback&, std::function<bool (libebml::EbmlElement const&)> const&, bool, bool) (lib/libebml/src/EbmlElement.cpp:495)
==2299633== by 0x3FB666: render_ebml_head (src/merge/output_control.cpp:494)
==2299633== by 0x3FB666: render_headers (???:583)
==2299633== by 0x3FB666: create_next_output_file() (???:1427)
==2299633== by 0x399EBD: main (src/merge/mkvmerge.cpp:3206)
==2299633== Address 0x0 is not stack'd, malloc'd or (recently) free'd
==2299633==
==2299633==
==2299633== Process terminating with default action of signal 11 (SIGSEGV): dumping core
==2299633== Access not within mapped region at address 0x0
==2299633== at 0x3CF1EB: IsAlwaysDeprecated (EbmlElement.h:283)
==2299633== by 0x3CF1EB: libebml::EbmlElement::WriteAll(libebml::EbmlElement const&) (EbmlElement.h:462)
==2299633== by 0x71CB34: libebml::EbmlElement::Render(libebml::IOCallback&, std::function<bool (libebml::EbmlElement const&)> const&, bool, bool) (lib/libebml/src/EbmlElement.cpp:495)
==2299633== by 0x3FB666: render_ebml_head (src/merge/output_control.cpp:494)
==2299633== by 0x3FB666: render_headers (???:583)
==2299633== by 0x3FB666: create_next_output_file() (???:1427)
==2299633== by 0x399EBD: main (src/merge/mkvmerge.cpp:3206)
==2299633== If you believe this happened as a result of a stack
==2299633== overflow in your program's main thread (unlikely but
==2299633== possible), you can try to increase the size of the
==2299633== main thread stack using the --main-stacksize= flag.
==2299633== The main thread stack size used in this run was 8388608.
==2299633==
==2299633== HEAP SUMMARY:
==2299633== in use at exit: 23,406,336 bytes in 5,805 blocks
==2299633== total heap usage: 21,768 allocs, 15,963 frees, 25,605,418 bytes allocated
==2299633==
==2299633== LEAK SUMMARY:
==2299633== definitely lost: 0 bytes in 0 blocks
==2299633== indirectly lost: 0 bytes in 0 blocks
==2299633== possibly lost: 33,123 bytes in 1,052 blocks
==2299633== still reachable: 23,373,213 bytes in 4,753 blocks
==2299633== suppressed: 0 bytes in 0 blocks
==2299633== Rerun with --leak-check=full to see details of leaked memory
==2299633==
==2299633== Use --track-origins=yes to see where uninitialised values come from
==2299633== For lists of detected and suppressed errors, rerun with: -s
==2299633== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 0 from 0)
zsh: segmentation fault (core dumped) valgrind --tool=memcheck --num-callers=12 mkvmerge -o v.mkv v.avi
|
It might need #228 because libebml currently uses reference to temporary objects in the EBML elements versions. |
|
Yeah I had the same idea a minute or two after posting. Still busy testing the EBML ID constructor change, though. |
|
Alright, there's no segfault anymore, but 80% of my test cases have different checksums. I'll have to investigate why. A quick look suggests that this is due to how Like I said, I'll have to investigate further; likely not before the next weekend, 'cause this is a whole lot of test cases to go through. |
|
No rush, although I think #211 is the better (but stricter) solution. They should give the same result though. |
|
BTW I started porting mkvalidator and mkclean to libebml/libmatroska 2.0. This is how I found out the usage of the profile was broken. But the current code seems OK with mkvalidator. I have some tests with checksum for mkclean, so once the port is working I can also do such tests. |
On top of #207 only the last commit "don't try to write elements that are deprecated" matters here.
The write filter allows to skip gracefully the elements that should never be written. Such elements are currently asserting in RenderData and return 0 there. This would likely break the internals as the written size no longer matches the UpdateSize value. Relying on the write filter fixes this as the same element will be skipped in UpdateSize and in Render(Data).
Eventually it would be nice to remove the RenderData overrides and just rely on this. However whoever implements a custom write filter (mkvtoolnix) should probably take this in consideration. If we remove the overrides we will allow writing deprecated elements in some circumstances, which is currently not possible.
The same system allows filtering out different level of min/max for a given render call.