Backport of fixes to release 0.21.2 #970
Conversation
This just makes life easier.
The command used to rename is also added to the minimization script, to avoid producing the same issue in future corpus directories and to have a reference if necessary.
Most uncomplete matches are removed by creating proper enums in the webp implementation and parsing to those constant variants early on. Some unsupported operations are explicitely failed with a Unsupported error variant.
Includes two seeds that were small enough to be viable. They are simply converted (with imagemagick) samples from the tests/images/ directory. This is good enough for the first few failures and panics. If you want a larger corpus, you can put other images into the directory although larger images can slow the fuzzer down without necessarily giving larger coverage.
HeroicKatora
force-pushed
the
version-0.21
branch
from
1172568 to
f087260
Compare
|
I think this plan makes a lot of sense. We have a bunch of breaking changes planned/merged on master so it is good to have one final release without them and a lingering branch in case there are any further patches we want to backport. |
This seems to originate from one of the samples provided in the RFC, specifically on page 23. A working version, that does the check for its input length, can be found in the attached implementation in section 20. This error is diagnosed as a FormatError.
A partition not containing as much data as claimed in the header is a format error. While ignoring it at that moment will likely lead to some later issues, it is better to check that the read has indeed taken place.
Simply do not refresh the cache state when bits are required during the decoding process than can be read into the decoder. This strategy is suggested in the reference implementation of the RFC.
At least one of the table entries can exceed the range of a signed 16-bit integer in an intermediate result. The reference implementation used an `int` of unspecified width for this but this is wasteful in terms of size of the table (albeit only slightly since the table is small).
Some of the operations can overflow in i32, according to debug mode builds. This is not intentional as the math is applied to conceptual fixed point numbers.
HeroicKatora
force-pushed
the
version-0.21
branch
from
f087260 to
da79c38
Compare
|
Seems like a good idea. This makes me wonder if we should have stricter commit policies in order to make this kind of work easier going forward? @HeroicKatora you should be the most qualified to to answer this, are unstructured/unclear commits a pain when doing this kind of work or is it fine? A quick look through the commit history seems like things are pretty clear, but we have a quite liberal policy with a lot of contributors with different styles. I guess some marker that differentiates commits with and without breaking changes could be useful? And a policy to keep bug-fixes separate from breaking changes if possible? I am not trying to say that I think people here write bad commits, just that we maybe with some kind of policy could make stuff like this easier to do. |
|
@birktj It was still alright. There are obviously things that could be improved but the commit style is manageable since almost all PRs were either bugfixes or feature additions. Maybe I missed something but a few additional commits should not be too painful to rebase into a I think the biggest lesson to learn here is that the nature of the repository, partially an aggregate of multiple independent image related projects, obviously promotes features and fixes arriving in mixed succession. But rather than purely require better commit messages (though guiding contributors to a style guide wouldn't be wrong) maybe we could have a
Something that might be helpful in communicating this: contributors theoretically have the power to amend and force push to the PR branch (by default, can be switched off explicitly). I don't think it's a good idea to normalize it but at the same time am not sure if contributors are typically aware of it or would be comfortable with a little help on wording and style? Checkbox in the template for opting into it could make this clear. |
Backports the fixes that happened on
masterto the0.21versionseries. Due to the pressure from memory-safety issues surrounding
the release of
0.21.1the master branch had already significantinterface changes applied. However,
crates.ioindicates that a significant amount of users lags behind in major versions
of the crate.
Reducing the ecosystem burden by trying to maintain at least this
version for a longer amount should give more users the chance to
update to a version that fixed the soundness issues. It should be
stressed that this does NOT remove the
PixelMuttype despiteits issues but 'only' rectifies internal issues It was already deprecated
and such a change absolutely requires a major version. And there are
no promises attached to how long this branch is maintained.
Release of the newer interface with
0.22, which removes thatoutstanding unsafe interface, will follow shortly.