Skip to content

Commit

Permalink
Merge remote-tracking branch 'upstream/version-0.21'
Browse files Browse the repository at this point in the history
  • Loading branch information
HeroicKatora committed Jun 11, 2019
2 parents 57d24a4 + c9afd64 commit 4a5438d
Show file tree
Hide file tree
Showing 4 changed files with 76 additions and 3 deletions.
21 changes: 20 additions & 1 deletion CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,29 @@ Rust image aims to be a pure-Rust implementation of various popular image format

## Changes

### Version 0.22 (unreleased)
### Version 0.22

- Support loading 2-bit BMP images

### Version 0.21.2

- Fixed a variety of crashes and opaque errors in webp
- Updated the png limits to be less restrictive
- Reworked even more `unsafe` operations into safe alternatives
- Derived Debug on FilterType and Deref on Pixel
- Removed a restriction on DXT to always require power of two dimensions
- Change the encoding of RGBA in bmp using bitfields
- Corrected various urls

### Version 0.21.1

- A fairly important bugfix backport
- Fixed a potentially memory safety issue in the hdr and tiff decoders, see #885
- See [the full advisory](docs/2019-04-23-memory-unsafety.md) for an analysis
- Fixes `ImageBuffer` index calculation for very, very large images
- Fix some crashes while parsing specific incomplete pnm images
- Added comprehensive fuzzing for the pam image types

### Version 0.21

- Updated README to use `GenericImageView`
Expand Down
2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "image"
version = "0.21.0"
version = "0.21.2"
license = "MIT"
description = "Imaging library written in Rust. Provides basic filters and decoders for the most common image formats."
authors = [
Expand Down
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ let pixel = img[(100, 100)];
let pixel = *img.get_pixel(100, 100);

// Put a pixel at coordinate (100, 100).
img.put_pixel(100, 100, pixel);
img.put_pixel(100, 100, *pixel);

// Iterate over all pixels in the image.
for pixel in img.pixels() {
Expand Down
54 changes: 54 additions & 0 deletions docs/2019-04-23-memory-unsafety.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
# Advisory about potential memory unsafety issues

[While reviewing][i885] some `unsafe Vec::from_raw_parts` operations within the
library, trying to justify their existence with stronger reasoning, we noticed
that they instead did not meet the required conditions set by the standard
library. This unsoundness was quickly removed, but we noted that the same
unjustified reasoning had been applied by a dependency introduced in `0.21`.

For efficiency reasons, we had tried to reuse the allocations made by decoders
for the buffer of the final image. However, that process is error prone. Most
image decoding algorithms change the representation type of color samples to
some degree. Notably, the output pixel type may have a different size and
alignment than the type used in the temporary decoding buffer. In this specific
instance, the `ImageBuffer` of the output expects a linear arrangement of `u8`
samples while the implementation of the `hdr` decoder uses a pixel
representation of `Rgb<u8>`, which has three times the size. One of the
requirements of `Vec::from_raw_parts` reads:

> ptr's T needs to have the same size and alignment as it was allocated with.
This requirement is not present on slices `[T]`, as it is motivated by the
allocator interface. The validity invariant of a reference and slice only
requires the correct alignment here, which was considered in the design of
`Rgb<_>` by giving it a well-defined representation, `#[repr(C)]`. But
critically, this does not guarantee that we can reuse the existing allocation
through effectively transmuting a `Vec<_>`!

The actual impact of this issue is, in real world implementations, limited to
allocators which handle allocations for types of size `1` and `3`/`4`
differently. To the best of my knowledge, this does not apply to `jemalloc` and
the `libc` allocator. However, we decided to proceed with caution here.

## Lessons for the future

New library dependencies will be under a stricter policy. Not only would they
need to be justified by functionality but also require at least some level of
reasoning how they solve that problem better than alternatives. Some appearance
of maintenance, or the existence of `#[deny(unsafe)]`, will help. We'll
additionally look into existing dependencies trying to identify similar issues
and minimizing the potential surface for implementation risks.

## Sound and safe buffer reuse

It seems that the `Vec` representation is entirely unfit for buffer reuse in
the style which an image library requires. In particular, using pixel types of
different sizes is likely common to handle either whole (encoded) pixels or
individual samples. Thus, we started a new sub-project to address this use
case, [image-canvas][image-canvas]. Contributions and review of its safety are
very welcome, we ask for the communities help here. The release of `v0.1` will
not occur until at least one such review has occurred.


[i885]: https://github.com/image-rs/image/pull/885
[image-canvas]: https://github.com/image-rs/canvas

0 comments on commit 4a5438d

Please sign in to comment.