Repository: J-F-Liu/lopdf (language: rust)

Issue:
Reduce `as usize` usage
The crate has a lot of code like this:
https://github.com/J-F-Liu/lopdf/blob/e635422ec66b546937f6484355bffe9b3f076264/src/object.rs#L652

In case of `Columns` being negative `as usize` will overflow and produce incorrect results.
Reduce `as usize` usage
The crate has a lot of code like this:
https://github.com/J-F-Liu/lopdf/blob/e635422ec66b546937f6484355bffe9b3f076264/src/object.rs#L652

In case of `Columns` being negative `as usize` will overflow and produce incorrect results.


Discussion / hints:

Fix patch:
diff --git a/Cargo.toml b/Cargo.toml
index 55a7a0f..890d464 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -40,5 +40,9 @@ pom_parser = ["pom"]
 name = "add_barcode"
 required-features = ["pom_parser"]
 
+[[tests]]
+name = "modify"
+required-features = ["pom_parser"]
+
 [badges]
 travis-ci = {repository = "J-F-Liu/lopdf"}
diff --git a/src/nom_parser.rs b/src/nom_parser.rs
index c160202..02c3f12 100644
--- a/src/nom_parser.rs
+++ b/src/nom_parser.rs
@@ -28,9 +28,10 @@ fn strip_nom<O>(r: NomResult<O>) -> Option<O> {
 
 #[inline]
 fn convert_result<O, E>(result: Result<O, E>, input: &[u8], error_kind: ErrorKind) -> NomResult<O> {
-    result
-        .map(|o| (input, o))
-        .map_err(|_| nom::Err::Error(NomError::from_error_kind(input, error_kind)))
+    result.map(|o| (input, o)).map_err(|_| {
+        NomError::from_error_kind(input, error_kind);
+        nom::Err::Error(())
+    })
 }
 
 #[inline]
@@ -149,14 +150,14 @@ fn escape_sequence(input: &[u8]) -> NomResult<Option<u8>> {
 enum ILS<'a> {
     Direct(&'a [u8]),
     Escape(Option<u8>),
-    EOL(&'a [u8]),
+    Eol(&'a [u8]),
     Nested(Vec<u8>),
 }
 
 impl<'a> ILS<'a> {
     fn push(&self, output: &mut Vec<u8>) {
         match self {
-            ILS::Direct(s) | ILS::EOL(s) => output.extend_from_slice(s),
+            ILS::Direct(s) | ILS::Eol(s) => output.extend_from_slice(s),
             ILS::Escape(e) => output.extend(e),
             ILS::Nested(n) => output.extend_from_slice(n),
         }
@@ -169,7 +170,7 @@ fn inner_literal_string(depth: usize) -> impl Fn(&[u8]) -> NomResult<Vec<u8>> {
             alt((
                 map(take_while1(is_direct_literal_string), ILS::Direct),
                 map(escape_sequence, ILS::Escape),
-                map(eol, ILS::EOL),
+                map(eol, ILS::Eol),
                 map(nested_literal_string(depth), ILS::Nested),
             )),
             Vec::new(),
@@ -276,6 +277,9 @@ fn stream<'a>(input: &'a [u8], reader: &Reader) -> NomResult<'a, Object> {
             value.as_i64()
         }
     }) {
+        if length < 0 {
+            return Err(nom::Err::Failure(()));
+        }
         let (i, data) = terminated(take(length as usize), pair(opt(eol), tag(b"endstream")))(i)?;
         Ok((i, Object::Stream(Stream::new(dict, data.to_vec()))))
     } else {
diff --git a/src/object.rs b/src/object.rs
index 439fd05..0b8461c 100644
--- a/src/object.rs
+++ b/src/object.rs
@@ -1,6 +1,7 @@
 use crate::{Document, Error, Result};
 use linked_hash_map::{self, Iter, IterMut, LinkedHashMap};
 use log::warn;
+use std::cmp::max;
 use std::fmt;
 use std::str;
 
@@ -629,9 +630,9 @@ impl Stream {
         if let Some(params) = params {
             let predictor = params.get(b"Predictor").and_then(Object::as_i64).unwrap_or(1);
             if (10..=15).contains(&predictor) {
-                let pixels_per_row = params.get(b"Columns").and_then(Object::as_i64).unwrap_or(1) as usize;
-                let colors = params.get(b"Colors").and_then(Object::as_i64).unwrap_or(1) as usize;
-                let bits = params.get(b"BitsPerComponent").and_then(Object::as_i64).unwrap_or(8) as usize;
+                let pixels_per_row = max(1, params.get(b"Columns").and_then(Object::as_i64).unwrap_or(1)) as usize;
+                let colors = max(1, params.get(b"Colors").and_then(Object::as_i64).unwrap_or(1)) as usize;
+                let bits = max(8, params.get(b"BitsPerComponent").and_then(Object::as_i64).unwrap_or(8)) as usize;
                 let bytes_per_pixel = colors * bits / 8;
                 data = png::decode_frame(data.as_slice(), bytes_per_pixel, pixels_per_row)?;
             }
diff --git a/src/object_stream.rs b/src/object_stream.rs
index e1a668f..7463667 100644
--- a/src/object_stream.rs
+++ b/src/object_stream.rs
@@ -2,6 +2,7 @@
 
 use crate::parser;
 use crate::{Error, Object, ObjectId, Result, Stream};
+use std::cmp::max;
 use std::collections::BTreeMap;
 use std::str::FromStr;
 
@@ -23,8 +24,8 @@ impl ObjectStream {
             });
         }
 
-        let first_offset = stream.dict.get(b"First").and_then(Object::as_i64)? as usize;
-        let _count = stream.dict.get(b"N").and_then(Object::as_i64)? as usize;
+        let first_offset = max(0, stream.dict.get(b"First").and_then(Object::as_i64)?) as usize;
+        let _count = max(0, stream.dict.get(b"N").and_then(Object::as_i64)?) as usize;
 
         let index_block = stream.content.get(..first_offset).ok_or(Error::Offset(first_offset))?;
 
diff --git a/src/parser.rs b/src/parser.rs
index 1813e47..1410da3 100644
--- a/src/parser.rs
+++ b/src/parser.rs
@@ -6,6 +6,7 @@ use crate::xref::*;
 use crate::{Error, Result};
 use pom::char_class::{alpha, hex_digit, multispace, oct_digit};
 use pom::parser::*;
+use std::cmp::max;
 use std::str::{self, FromStr};
 
 fn eol<'a>() -> Parser<'a, u8, u8> {
@@ -221,11 +222,11 @@ pub fn header(input: &[u8]) -> Option<String> {
 
 /// Decode CrossReferenceTable
 fn xref<'a>() -> Parser<'a, u8, Xref> {
-    let xref_entry = integer().map(|i| i as u32) - sym(b' ') + integer().map(|i| i as u16) - sym(b' ')
+    let xref_entry = integer().map(|i| max(0, i) as u32) - sym(b' ') + integer().map(|i| i as u16) - sym(b' ')
         + one_of(b"nf").map(|k| k == b'n')
         - take(2);
-    let xref_section =
-        integer().map(|i| i as usize) - sym(b' ') + integer() - sym(b' ').opt() - eol() + xref_entry.repeat(0..);
+    let xref_section = integer().map(|i| max(0, i) as usize) - sym(b' ') + integer() - sym(b' ').opt() - eol()
+        + xref_entry.repeat(0..);
     let xref = seq(b"xref") * sym(b' ').opt() * eol() * xref_section.repeat(1..) - space();
     xref.map(|sections| {
         sections.into_iter().fold(
diff --git a/src/parser_aux.rs b/src/parser_aux.rs
index a461313..f14bd9a 100644
--- a/src/parser_aux.rs
+++ b/src/parser_aux.rs
@@ -96,9 +96,10 @@ impl Document {
     }
 
     pub fn replace_text(&mut self, page_number: u32, text: &str, other_text: &str) -> Result<()> {
+        let page = page_number.saturating_sub(1) as usize;
         let page_id = self
             .page_iter()
-            .nth(page_number as usize - 1)
+            .nth(page)
             .ok_or(Error::PageNumberNotFound(page_number))?;
         let encodings = self
             .get_page_fonts(page_id)
@@ -204,7 +205,11 @@ pub fn decode_xref_stream(mut stream: Stream) -> Result<(Xref, Dictionary)> {
             .and_then(parse_integer_array)
             .map_err(|_| Error::Xref(XrefError::Parse))?;
 
-        if field_widths.len() < 3 {
+        if field_widths.len() < 3
+            || field_widths[0].is_negative()
+            || field_widths[1].is_negative()
+            || field_widths[2].is_negative()
+        {
             return Err(Error::Xref(XrefError::Parse));
         }
 
diff --git a/src/reader.rs b/src/reader.rs
index ec87ec7..213441c 100644
--- a/src/reader.rs
+++ b/src/reader.rs
@@ -136,21 +136,21 @@ impl<'a> Reader<'a> {
         // Read previous Xrefs of linearized or incremental updated document.
         let mut prev_xref_start = trailer.remove(b"Prev");
         while let Some(prev) = prev_xref_start.and_then(|offset| offset.as_i64().ok()) {
-            let prev = prev as usize;
-            if prev > self.buffer.len() {
+            if prev < 0 || prev as usize > self.buffer.len() {
                 return Err(Error::Xref(XrefError::PrevStart));
             }
-            let (prev_xref, mut prev_trailer) = parser::xref_and_trailer(&self.buffer[prev..], &self)?;
+
+            let (prev_xref, mut prev_trailer) = parser::xref_and_trailer(&self.buffer[prev as usize..], &self)?;
             xref.merge(prev_xref);
 
             // Read xref stream in hybrid-reference file
             let prev_xref_stream_start = trailer.remove(b"XRefStm");
             if let Some(prev) = prev_xref_stream_start.and_then(|offset| offset.as_i64().ok()) {
-                let prev = prev as usize;
-                if prev > self.buffer.len() {
+                if prev < 0 || prev as usize > self.buffer.len() {
                     return Err(Error::Xref(XrefError::StreamStart));
                 }
-                let (prev_xref, _) = parser::xref_and_trailer(&self.buffer[prev..], &self)?;
+
+                let (prev_xref, _) = parser::xref_and_trailer(&self.buffer[prev as usize..], &self)?;
                 xref.merge(prev_xref);
             }
 


Test patch:
diff --git a/tests/modify.rs b/tests/modify.rs
index fe6e994..8b92ba2 100644
--- a/tests/modify.rs
+++ b/tests/modify.rs
@@ -15,8 +15,12 @@ fn test_get_object() {
 
     println!("{:?}", id);
     println!("{:?}", id2);
-    assert!(doc.get_object(id).is_ok());
-    assert!(doc.get_object(id2).is_ok());
+
+    let obj1_exists = doc.get_object(id).is_ok();
+    let obj2_exists = doc.get_object(id2).is_ok();
+
+    assert!(obj1_exists);
+    assert!(obj2_exists);
 }
 
 #[cfg(any(feature = "pom_parser", feature = "nom_parser"))]
@@ -24,7 +28,7 @@ mod tests_with_parsing {
     use super::*;
     use lopdf::Result;
 
-    fn modify_text() -> Result<Document> {
+    fn modify_text() -> Result<bool> {
         let mut doc = Document::load("assets/example.pdf")?;
         doc.version = "1.4".to_string();
         if let Some(Object::Stream(ref mut stream)) = doc.objects.get_mut(&(4, 0)) {
@@ -37,12 +41,12 @@ mod tests_with_parsing {
         let temp_dir = tempfile::tempdir()?;
         let file_path = temp_dir.path().join("test_3_modify.pdf");
         doc.save(file_path)?;
-        Ok(doc)
+        Ok(true)
     }
 
     #[test]
     fn test_modify() {
-        assert!(modify_text().is_ok());
+        assert!(modify_text().unwrap());
     }
 
     fn replace_text() -> Result<Document> {
@@ -63,7 +67,7 @@ mod tests_with_parsing {
         assert_eq!(replace_text().unwrap().extract_text(&[1]).unwrap(), "Modified text!\n");
     }
 
-    fn get_mut() -> Result<()> {
+    fn get_mut() -> Result<bool> {
         let mut doc = Document::load("assets/example.pdf")?;
         let arr = doc
             .get_object_mut((5, 0))?
@@ -71,11 +75,11 @@ mod tests_with_parsing {
             .get_mut(b"Contents")?
             .as_array_mut()?;
         arr[0] = arr[0].clone();
-        Ok(())
+        Ok(true)
     }
 
     #[test]
     fn test_get_mut() {
-        assert!(get_mut().is_ok());
+        assert!(get_mut().unwrap());
     }
 }