From eadec8f0b960027de82203137f0d86ecd75be714 Mon Sep 17 00:00:00 2001 From: Andrew Champion Date: Fri, 20 Dec 2019 17:00:14 +0000 Subject: [PATCH] Remove most unchecked size/index casts Convert most unchecked size/index `as` casts to checked casts with `TryFrom`, included another top-level error variant. This has several motivations: - Making the library as robust as possible, including to platform differences. - Unchecked casts are susceptible to breakage during refactorings. - Preparing for BigTIFF support, which will require the introduction of generics and associated types for some parts of the de/encoders. Unchecked primitive casts cannot be bound, while TryFrom can, and overflowing conversions become more likely. - Remaining u16 casts may be prone in failure in existing practice and not just in theory during future changes. --- src/decoder/ifd.rs | 11 ++++--- src/decoder/mod.rs | 53 +++++++++++++++++----------------- src/encoder/mod.rs | 65 ++++++++++++++++++++++-------------------- src/error.rs | 12 ++++++++ tests/encode_images.rs | 40 +++++++++++++------------- 5 files changed, 100 insertions(+), 81 deletions(-) diff --git a/src/decoder/ifd.rs b/src/decoder/ifd.rs index 73b6e97..58965d1 100644 --- a/src/decoder/ifd.rs +++ b/src/decoder/ifd.rs @@ -1,6 +1,7 @@ //! Function for reading TIFF tags use std::collections::HashMap; +use std::convert::TryFrom; use std::io::{self, Read, Seek}; use std::mem; @@ -193,11 +194,12 @@ impl Entry { )) }), (Type::ASCII, n) => { - if n as usize > limits.decoding_buffer_size { + let n = usize::try_from(n)?; + if n > limits.decoding_buffer_size { return Err(TiffError::LimitsExceeded); } decoder.goto_offset(self.r(bo).read_u32()?)?; - let string = decoder.read_string(n as usize)?; + let string = decoder.read_string(n)?; Ok(Ascii(string)) } _ => Err(TiffError::UnsupportedError( @@ -212,11 +214,12 @@ impl Entry { R: Read + Seek, F: Fn(&mut super::Decoder) -> TiffResult, { - if value_count as usize > limits.decoding_buffer_size / mem::size_of::() { + let value_count = usize::try_from(value_count)?; + if value_count > limits.decoding_buffer_size / mem::size_of::() { return Err(TiffError::LimitsExceeded); } - let mut v = Vec::with_capacity(value_count as usize); + let mut v = Vec::with_capacity(value_count); decoder.goto_offset(self.r(bo).read_u32()?)?; for _ in 0..value_count { v.push(decode_fn(decoder)?) diff --git a/src/decoder/mod.rs b/src/decoder/mod.rs index 30ee3f1..9d06c31 100644 --- a/src/decoder/mod.rs +++ b/src/decoder/mod.rs @@ -197,12 +197,12 @@ impl Wrapping for u64 { } } -fn rev_hpredict_nsamp(image: &mut [T], size: (u32, u32), samples: usize) +fn rev_hpredict_nsamp(image: &mut [T], size: (u32, u32), samples: usize) -> TiffResult<()> where T: Copy + Wrapping, { - let width = size.0 as usize; - let height = size.1 as usize; + let width = usize::try_from(size.0)?; + let height = usize::try_from(size.1)?; for row in 0..height { for col in samples..width * samples { let prev_pixel = image[(row * width * samples + col - samples)]; @@ -210,6 +210,7 @@ where *pixel = pixel.wrapping_add(prev_pixel); } } + Ok(()) } fn rev_hpredict(image: DecodingBuffer, size: (u32, u32), color_type: ColorType) -> TiffResult<()> { @@ -226,16 +227,16 @@ fn rev_hpredict(image: DecodingBuffer, size: (u32, u32), color_type: ColorType) }; match image { DecodingBuffer::U8(buf) => { - rev_hpredict_nsamp(buf, size, samples); + rev_hpredict_nsamp(buf, size, samples)?; } DecodingBuffer::U16(buf) => { - rev_hpredict_nsamp(buf, size, samples); + rev_hpredict_nsamp(buf, size, samples)?; } DecodingBuffer::U32(buf) => { - rev_hpredict_nsamp(buf, size, samples); + rev_hpredict_nsamp(buf, size, samples)?; } DecodingBuffer::U64(buf) => { - rev_hpredict_nsamp(buf, size, samples); + rev_hpredict_nsamp(buf, size, samples)?; } } Ok(()) @@ -575,14 +576,14 @@ impl Decoder { CompressionMethod::None => { let order = self.reader.byte_order; ( - length as usize, + usize::try_from(length)?, Box::new(SmartReader::wrap(&mut self.reader, order)), ) } CompressionMethod::LZW => { let (bytes, reader) = LZWReader::new( &mut self.reader, - length as usize, + usize::try_from(length)?, max_uncompressed_length )?; (bytes, Box::new(reader)) @@ -592,7 +593,7 @@ impl Decoder { let (bytes, reader) = PackBitsReader::new( &mut self.reader, order, - length as usize + usize::try_from(length)?, )?; (bytes, Box::new(reader)) } @@ -732,18 +733,18 @@ impl Decoder { TiffFormatError::InconsistentSizesEncountered, ))?; - let rows_per_strip = self + let rows_per_strip = usize::try_from(self .get_tag_u32(Tag::RowsPerStrip) - .unwrap_or(self.height) as usize; + .unwrap_or(self.height))?; let strip_height = cmp::min( rows_per_strip, - self.height as usize - index * rows_per_strip, + usize::try_from(self.height)? - index * rows_per_strip, ); - let buffer_size = self.width as usize * strip_height * self.bits_per_sample.len(); + let buffer_size = usize::try_from(self.width)? * strip_height * self.bits_per_sample.len(); - if buffer.len() < buffer_size || byte_count as usize / buffer.byte_len() > buffer_size { + if buffer.len() < buffer_size || usize::try_from(byte_count)? / buffer.byte_len() > buffer_size { return Err(TiffError::FormatError( TiffFormatError::InconsistentSizesEncountered, )); @@ -753,7 +754,7 @@ impl Decoder { self.strip_decoder.as_mut().unwrap().strip_index += 1; - if index as u32 == self.strip_count()? { + if u32::try_from(index)? == self.strip_count()? { self.strip_decoder = None; } @@ -768,7 +769,7 @@ impl Decoder { Some(Predictor::Horizontal) => { rev_hpredict( buffer.copy(), - (self.width, strip_height as u32), + (self.width, u32::try_from(strip_height)?), self.colortype()?, )?; } @@ -784,7 +785,7 @@ impl Decoder { } fn result_buffer(&self, height: usize) -> TiffResult { - let buffer_size = self.width as usize * height * self.bits_per_sample.len(); + let buffer_size = usize::try_from(self.width)? * height * self.bits_per_sample.len(); match self.bits_per_sample.iter().cloned().max().unwrap_or(8) { n if n <= 8 => DecodingResult::new_u8(buffer_size, &self.limits), @@ -804,13 +805,13 @@ impl Decoder { self.initialize_strip_decoder()?; let index = self.strip_decoder.as_ref().unwrap().strip_index; - let rows_per_strip = self + let rows_per_strip = usize::try_from(self .get_tag_u32(Tag::RowsPerStrip) - .unwrap_or(self.height) as usize; + .unwrap_or(self.height))?; let strip_height = cmp::min( rows_per_strip, - self.height as usize - index * rows_per_strip, + usize::try_from(self.height)? - index * rows_per_strip, ); let mut result = self.result_buffer(strip_height)?; @@ -823,16 +824,16 @@ impl Decoder { /// Decodes the entire image and return it as a Vector pub fn read_image(&mut self) -> TiffResult { self.initialize_strip_decoder()?; - let rows_per_strip = self + let rows_per_strip = usize::try_from(self .get_tag_u32(Tag::RowsPerStrip) - .unwrap_or(self.height) as usize; + .unwrap_or(self.height))?; let samples_per_strip = - self.width as usize * rows_per_strip * self.bits_per_sample.len(); + usize::try_from(self.width)? * rows_per_strip * self.bits_per_sample.len(); - let mut result = self.result_buffer(self.height as usize)?; + let mut result = self.result_buffer(usize::try_from(self.height)?)?; - for i in 0..self.strip_count()? as usize { + for i in 0..usize::try_from(self.strip_count()?)? { self.read_strip_to_buffer(result.as_buffer(samples_per_strip * i))?; } diff --git a/src/encoder/mod.rs b/src/encoder/mod.rs index 27e3a37..e3d7e0d 100644 --- a/src/encoder/mod.rs +++ b/src/encoder/mod.rs @@ -1,5 +1,6 @@ use byteorder::NativeEndian; use std::collections::BTreeMap; +use std::convert::TryFrom; use std::io::{Seek, Write}; use std::mem; @@ -406,7 +407,7 @@ impl TiffEncoder { where [C::Inner]: TiffValue, { - let num_pix = (width as usize).checked_mul(height as usize) + let num_pix = usize::try_from(width)?.checked_mul(usize::try_from(height)?) .ok_or_else(|| ::std::io::Error::new( ::std::io::ErrorKind::InvalidInput, "Image width * height exceeds usize"))?; @@ -421,7 +422,7 @@ impl TiffEncoder { let mut idx = 0; while image.next_strip_sample_count() > 0 { - let sample_count = image.next_strip_sample_count() as usize; + let sample_count = usize::try_from(image.next_strip_sample_count())?; image.write_strip(&data[idx..idx + sample_count])?; idx += sample_count; } @@ -444,7 +445,7 @@ pub struct DirectoryEncoder<'a, W: 'a + Write + Seek> { impl<'a, W: 'a + Write + Seek> DirectoryEncoder<'a, W> { fn new(writer: &'a mut TiffWriter) -> TiffResult> { // the previous word is the IFD offset position - let ifd_pointer_pos = writer.offset() - mem::size_of::() as u64; + let ifd_pointer_pos = writer.offset() - u64::try_from(mem::size_of::())?; writer.pad_word_boundary()?; Ok(DirectoryEncoder { writer, @@ -455,16 +456,18 @@ impl<'a, W: 'a + Write + Seek> DirectoryEncoder<'a, W> { } /// Write a single ifd tag. - pub fn write_tag(&mut self, tag: Tag, value: T) { + pub fn write_tag(&mut self, tag: Tag, value: T) -> TiffResult<()> { let len = ::BYTE_LEN * value.count(); - let mut bytes = Vec::with_capacity(len as usize); + let mut bytes = Vec::with_capacity(usize::try_from(len)?); { let mut writer = TiffWriter::new(&mut bytes); - value.write(&mut writer).unwrap(); + value.write(&mut writer)?; } self.ifd .insert(tag.to_u16(), (::FIELD_TYPE.to_u16(), value.count(), bytes)); + + Ok(()) } fn write_directory(&mut self) -> TiffResult { @@ -475,7 +478,7 @@ impl<'a, W: 'a + Write + Seek> DirectoryEncoder<'a, W> { self.writer.write_bytes(bytes)?; *bytes = vec![0, 0, 0, 0]; let mut writer = TiffWriter::new(bytes as &mut [u8]); - writer.write_u32(offset as u32)?; + writer.write_u32(u32::try_from(offset)?)?; } else { while bytes.len() < 4 { bytes.push(0); @@ -485,7 +488,7 @@ impl<'a, W: 'a + Write + Seek> DirectoryEncoder<'a, W> { let offset = self.writer.offset(); - self.writer.write_u16(self.ifd.len() as u16)?; + self.writer.write_u16(u16::try_from(self.ifd.len())?)?; for (tag, &(ref field_type, ref count, ref offset)) in self.ifd.iter() { self.writer.write_u16(*tag)?; self.writer.write_u16(*field_type)?; @@ -510,7 +513,7 @@ impl<'a, W: 'a + Write + Seek> DirectoryEncoder<'a, W> { let curr_pos = self.writer.offset(); self.writer.goto_offset(self.ifd_pointer_pos)?; - self.writer.write_u32(ifd_pointer as u32)?; + self.writer.write_u32(u32::try_from(ifd_pointer)?)?; self.writer.goto_offset(curr_pos)?; self.writer.write_u32(0)?; @@ -577,7 +580,7 @@ impl<'a, W: 'a + Write + Seek, T: ColorType> ImageEncoder<'a, W, T> { width: u32, height: u32, ) -> TiffResult> { - let row_samples = u64::from(width) * ::BITS_PER_SAMPLE.len() as u64; + let row_samples = u64::from(width) * u64::try_from(::BITS_PER_SAMPLE.len())?; let row_bytes = row_samples * u64::from(::BYTE_LEN); // As per tiff spec each strip should be about 8k long @@ -585,19 +588,19 @@ impl<'a, W: 'a + Write + Seek, T: ColorType> ImageEncoder<'a, W, T> { let strip_count = (u64::from(height) + rows_per_strip - 1) / rows_per_strip; - encoder.write_tag(Tag::ImageWidth, width); - encoder.write_tag(Tag::ImageLength, height); - encoder.write_tag(Tag::Compression, tags::CompressionMethod::None.to_u16()); + encoder.write_tag(Tag::ImageWidth, width)?; + encoder.write_tag(Tag::ImageLength, height)?; + encoder.write_tag(Tag::Compression, tags::CompressionMethod::None.to_u16())?; - encoder.write_tag(Tag::BitsPerSample, ::BITS_PER_SAMPLE); - encoder.write_tag(Tag::PhotometricInterpretation, ::TIFF_VALUE.to_u16()); + encoder.write_tag(Tag::BitsPerSample, ::BITS_PER_SAMPLE)?; + encoder.write_tag(Tag::PhotometricInterpretation, ::TIFF_VALUE.to_u16())?; - encoder.write_tag(Tag::RowsPerStrip, rows_per_strip as u32); + encoder.write_tag(Tag::RowsPerStrip, u32::try_from(rows_per_strip)?)?; - encoder.write_tag(Tag::SamplesPerPixel, ::BITS_PER_SAMPLE.len() as u16); - encoder.write_tag(Tag::XResolution, Rational { n: 1, d: 1 }); - encoder.write_tag(Tag::YResolution, Rational { n: 1, d: 1 }); - encoder.write_tag(Tag::ResolutionUnit, ResolutionUnit::None.to_u16()); + encoder.write_tag(Tag::SamplesPerPixel, u16::try_from(::BITS_PER_SAMPLE.len())?)?; + encoder.write_tag(Tag::XResolution, Rational { n: 1, d: 1 })?; + encoder.write_tag(Tag::YResolution, Rational { n: 1, d: 1 })?; + encoder.write_tag(Tag::ResolutionUnit, ResolutionUnit::None.to_u16())?; Ok(ImageEncoder { encoder, @@ -635,15 +638,15 @@ impl<'a, W: 'a + Write + Seek, T: ColorType> ImageEncoder<'a, W, T> { { // TODO: Compression let samples = self.next_strip_sample_count(); - if value.len() as u64 != samples { + if u64::try_from(value.len())? != samples { return Err(::std::io::Error::new( ::std::io::ErrorKind::InvalidData, "Slice is wrong size for strip").into()); } let offset = self.encoder.write_data(value)?; - self.strip_offsets.push(offset as u32); - self.strip_byte_count.push(value.bytes() as u32); + self.strip_offsets.push(u32::try_from(offset)?); + self.strip_byte_count.push(value.bytes()); self.strip_idx += 1; Ok(()) @@ -651,31 +654,31 @@ impl<'a, W: 'a + Write + Seek, T: ColorType> ImageEncoder<'a, W, T> { /// Set image resolution pub fn resolution(&mut self, unit: ResolutionUnit, value: Rational) { - self.encoder.write_tag(Tag::ResolutionUnit, unit.to_u16()); - self.encoder.write_tag(Tag::XResolution, value.clone()); - self.encoder.write_tag(Tag::YResolution, value); + self.encoder.write_tag(Tag::ResolutionUnit, unit.to_u16()).unwrap(); + self.encoder.write_tag(Tag::XResolution, value.clone()).unwrap(); + self.encoder.write_tag(Tag::YResolution, value).unwrap(); } /// Set image resolution unit pub fn resolution_unit(&mut self, unit: ResolutionUnit) { - self.encoder.write_tag(Tag::ResolutionUnit, unit.to_u16()); + self.encoder.write_tag(Tag::ResolutionUnit, unit.to_u16()).unwrap(); } /// Set image x-resolution pub fn x_resolution(&mut self, value: Rational) { - self.encoder.write_tag(Tag::XResolution, value); + self.encoder.write_tag(Tag::XResolution, value).unwrap(); } /// Set image y-resolution pub fn y_resolution(&mut self, value: Rational) { - self.encoder.write_tag(Tag::YResolution, value); + self.encoder.write_tag(Tag::YResolution, value).unwrap(); } fn finish_internal(&mut self) -> TiffResult<()> { self.encoder - .write_tag(Tag::StripOffsets, &*self.strip_offsets); + .write_tag(Tag::StripOffsets, &*self.strip_offsets)?; self.encoder - .write_tag(Tag::StripByteCounts, &*self.strip_byte_count); + .write_tag(Tag::StripByteCounts, &*self.strip_byte_count)?; self.dropped = true; self.encoder.finish_internal() diff --git a/src/error.rs b/src/error.rs index 27ef8c3..02acbe0 100644 --- a/src/error.rs +++ b/src/error.rs @@ -22,6 +22,10 @@ pub enum TiffError { /// The Limits of the Decoder is exceeded. LimitsExceeded, + + /// An integer conversion to or from a platform size failed, either due to + /// limits of the platform size or limits of the format. + IntSizeError, } /// The image is not formatted properly. @@ -171,6 +175,7 @@ impl fmt::Display for TiffError { ), TiffError::IoError(ref e) => e.fmt(fmt), TiffError::LimitsExceeded => write!(fmt, "The Decoder limits are exceeded"), + TiffError::IntSizeError => write!(fmt, "Platform or format size limits exceeded"), } } } @@ -182,6 +187,7 @@ impl Error for TiffError { TiffError::UnsupportedError(..) => "Unsupported error", TiffError::IoError(..) => "IO error", TiffError::LimitsExceeded => "Decoder limits exceeded", + TiffError::IntSizeError => "Platform or format size limits exceeded", } } @@ -217,5 +223,11 @@ impl From for TiffError { } } +impl From for TiffError { + fn from(_err: std::num::TryFromIntError) -> TiffError { + TiffError::IntSizeError + } +} + /// Result of an image decoding/encoding process pub type TiffResult = Result; diff --git a/tests/encode_images.rs b/tests/encode_images.rs index 32b1e88..c40fac3 100644 --- a/tests/encode_images.rs +++ b/tests/encode_images.rs @@ -144,11 +144,11 @@ fn test_multiple_byte() { let mut image_encoder = tiff.new_image::(1, 1).unwrap(); let encoder = image_encoder.encoder(); - encoder.write_tag(Tag::Unknown(65000), &[1_u8][..]); - encoder.write_tag(Tag::Unknown(65001), &[1_u8, 2][..]); - encoder.write_tag(Tag::Unknown(65002), &[1_u8, 2, 3][..]); - encoder.write_tag(Tag::Unknown(65003), &[1_u8, 2, 3, 4][..]); - encoder.write_tag(Tag::Unknown(65004), &[1_u8, 2, 3, 4, 5][..]); + encoder.write_tag(Tag::Unknown(65000), &[1_u8][..]).unwrap(); + encoder.write_tag(Tag::Unknown(65001), &[1_u8, 2][..]).unwrap(); + encoder.write_tag(Tag::Unknown(65002), &[1_u8, 2, 3][..]).unwrap(); + encoder.write_tag(Tag::Unknown(65003), &[1_u8, 2, 3, 4][..]).unwrap(); + encoder.write_tag(Tag::Unknown(65004), &[1_u8, 2, 3, 4, 5][..]).unwrap(); } data.set_position(0); @@ -174,24 +174,24 @@ fn test_signed() { let encoder = image_encoder.encoder(); //Use the "reusable" tags section as per the TIFF6 spec - encoder.write_tag(Tag::Unknown(65000), -1_i8); - encoder.write_tag(Tag::Unknown(65001), &[-1_i8][..]); - encoder.write_tag(Tag::Unknown(65002), &[-1_i8, 2][..]); - encoder.write_tag(Tag::Unknown(65003), &[-1_i8, 2, -3][..]); - encoder.write_tag(Tag::Unknown(65004), &[-1_i8, 2, -3, 4][..]); - encoder.write_tag(Tag::Unknown(65005), &[-1_i8, 2, -3, 4, -5][..]); + encoder.write_tag(Tag::Unknown(65000), -1_i8).unwrap(); + encoder.write_tag(Tag::Unknown(65001), &[-1_i8][..]).unwrap(); + encoder.write_tag(Tag::Unknown(65002), &[-1_i8, 2][..]).unwrap(); + encoder.write_tag(Tag::Unknown(65003), &[-1_i8, 2, -3][..]).unwrap(); + encoder.write_tag(Tag::Unknown(65004), &[-1_i8, 2, -3, 4][..]).unwrap(); + encoder.write_tag(Tag::Unknown(65005), &[-1_i8, 2, -3, 4, -5][..]).unwrap(); - encoder.write_tag(Tag::Unknown(65010), -1_i16); - encoder.write_tag(Tag::Unknown(65011), -1_i16); - encoder.write_tag(Tag::Unknown(65012), &[-1_i16, 2][..]); - encoder.write_tag(Tag::Unknown(65013), &[-1_i16, 2, -3][..]); + encoder.write_tag(Tag::Unknown(65010), -1_i16).unwrap(); + encoder.write_tag(Tag::Unknown(65011), -1_i16).unwrap(); + encoder.write_tag(Tag::Unknown(65012), &[-1_i16, 2][..]).unwrap(); + encoder.write_tag(Tag::Unknown(65013), &[-1_i16, 2, -3][..]).unwrap(); - encoder.write_tag(Tag::Unknown(65020), -1_i32); - encoder.write_tag(Tag::Unknown(65021), &[-1_i32][..]); - encoder.write_tag(Tag::Unknown(65022), &[-1_i32, 2][..]); + encoder.write_tag(Tag::Unknown(65020), -1_i32).unwrap(); + encoder.write_tag(Tag::Unknown(65021), &[-1_i32][..]).unwrap(); + encoder.write_tag(Tag::Unknown(65022), &[-1_i32, 2][..]).unwrap(); - encoder.write_tag(Tag::Unknown(65030), SRational { n: -1, d: 100 }); - encoder.write_tag(Tag::Unknown(65031), &[SRational { n: -1, d: 100 }, SRational { n: 2, d: 100 }][..]); + encoder.write_tag(Tag::Unknown(65030), SRational { n: -1, d: 100 }).unwrap(); + encoder.write_tag(Tag::Unknown(65031), &[SRational { n: -1, d: 100 }, SRational { n: 2, d: 100 }][..]).unwrap(); } //Rewind the cursor for reading