age: Allow armor with no newline after end marker

Fixes test failures for the following testkit test files:
- armor_garbage_encoded (async)
- armor_no_end_line (async)
- armor_no_eol

Cargo.lock

@@ -77,6 +77,7 @@ dependencies = [
  "i18n-embed",
  "i18n-embed-fl",
  "lazy_static",
+ "memchr",
  "nom",
  "num-traits",
  "pin-project",

age/CHANGELOG.md

@@ -17,6 +17,12 @@ to 1.0.0 are beta releases.
 - `age::Decryptor` now rejects invalid or non-canonical `scrypt` recipient
   stanzas (instead of ignoring or accepting them respectively), matching the
   [age specification](https://c2sp.org/age#scrypt-recipient-stanza).
+- `age::armor::ArmoredReader`:
+  - It now accepts armored files with no newline after the end marker.
+    Previously these were rejected by the synchronous API, and would cause the
+    async API to hang.
+  - The async API now correctly rejects some classes of invalid armoring that
+    previously would cause it to hang.
 
 ## [0.8.1] - 2022-06-18
 ### Security

age/Cargo.toml

@@ -68,6 +68,7 @@ zeroize = "1"
 
 # Async I/O
 futures = { version = "0.3", optional = true }
+memchr = { version = "2.5", optional = true }
 pin-project = "1"
 
 # Localization
@@ -108,7 +109,7 @@ criterion-cycles-per-byte = "0.1"
 [features]
 default = []
 armor = []
-async = ["futures"]
+async = ["futures", "memchr"]
 cli-common = ["atty", "console", "pinentry", "rpassword"]
 plugin = ["age-core/plugin", "which", "wsl"]
 ssh = [

age/src/primitives/armor.rs

@@ -16,7 +16,11 @@ use futures::{
     task::{Context, Poll},
 };
 #[cfg(feature = "async")]
+use std::mem;
+#[cfg(feature = "async")]
 use std::pin::Pin;
+#[cfg(feature = "async")]
+use std::str;
 
 const ARMORED_COLUMNS_PER_LINE: usize = 64;
 const ARMORED_BYTES_PER_LINE: usize = ARMORED_COLUMNS_PER_LINE / 4 * 3;
@@ -525,13 +529,6 @@ pub enum ArmoredReadError {
     InvalidUtf8,
     /// A line of the armor contains a `\r` character.
     LineContainsCr,
-    /// A line of the armor is missing a line ending.
-    ///
-    /// In practice, this only enforces a newline after the end marker, because the parser
-    /// splits the input on newlines, so a missing line ending internally would instead be
-    /// interpreted as either `ArmoredReadError::LineContainsCr` or
-    /// `ArmoredReadError::NotWrappedAt64Chars`.
-    MissingLineEnding,
     /// The armor is not wrapped at 64 characters.
     NotWrappedAt64Chars,
     /// There is a short line in the middle of the armor (only the final line may be short).
@@ -545,7 +542,6 @@ impl fmt::Display for ArmoredReadError {
             ArmoredReadError::InvalidBeginMarker => write!(f, "invalid armor begin marker"),
             ArmoredReadError::InvalidUtf8 => write!(f, "stream did not contain valid UTF-8"),
             ArmoredReadError::LineContainsCr => write!(f, "line contains CR"),
-            ArmoredReadError::MissingLineEnding => write!(f, "missing line ending"),
             ArmoredReadError::NotWrappedAt64Chars => {
                 write!(f, "invalid armor (not wrapped at 64 characters)")
             }
@@ -737,10 +733,11 @@ impl<R> ArmoredReader<R> {
         } else if self.line_buf.ends_with('\n') {
             self.line_buf.trim_end_matches('\n')
         } else {
-            return Err(io::Error::new(
-                io::ErrorKind::InvalidData,
-                ArmoredReadError::MissingLineEnding,
-            ));
+            // If the line does not end in a `\n`, then it must be the final line in the
+            // file, because we parse the file into lines by splitting on `\n`. This will
+            // either be an invalid line (and be caught as a different error), or the end
+            // marker (which we allow to omit a trailing `\n`).
+            &self.line_buf
         };
         if line.contains('\r') {
             return Err(io::Error::new(
@@ -847,6 +844,61 @@ impl<R: BufRead> Read for ArmoredReader<R> {
     }
 }
 
+/// Copied from `futures_util::io::read_until::read_until_internal`.
+#[cfg(feature = "async")]
+fn read_until_internal<R: AsyncBufRead + ?Sized>(
+    mut reader: Pin<&mut R>,
+    cx: &mut Context<'_>,
+    byte: u8,
+    buf: &mut Vec<u8>,
+    read: &mut usize,
+) -> Poll<io::Result<usize>> {
+    loop {
+        let (done, used) = {
+            let available = ready!(reader.as_mut().poll_fill_buf(cx))?;
+            if let Some(i) = memchr::memchr(byte, available) {
+                buf.extend_from_slice(&available[..=i]);
+                (true, i + 1)
+            } else {
+                buf.extend_from_slice(available);
+                (false, available.len())
+            }
+        };
+        reader.as_mut().consume(used);
+        *read += used;
+        if done || used == 0 {
+            return Poll::Ready(Ok(mem::replace(read, 0)));
+        }
+    }
+}
+
+/// Adapted from `futures_util::io::read_line::read_line_internal`.
+#[cfg(feature = "async")]
+fn read_line_internal<R: AsyncBufRead + ?Sized>(
+    reader: Pin<&mut R>,
+    cx: &mut Context<'_>,
+    buf: &mut String,
+    bytes: &mut Vec<u8>,
+    read: &mut usize,
+) -> Poll<io::Result<usize>> {
+    let ret = ready!(read_until_internal(reader, cx, b'\n', bytes, read));
+    match String::from_utf8(mem::take(bytes)) {
+        Err(_) => Poll::Ready(ret.and_then(|_| {
+            Err(io::Error::new(
+                io::ErrorKind::InvalidData,
+                ArmoredReadError::InvalidUtf8,
+            ))
+        })),
+        Ok(mut line) => {
+            debug_assert!(buf.is_empty());
+            debug_assert_eq!(*read, 0);
+            // Safety: `bytes` is a valid UTF-8 because `str::from_utf8` returned `Ok`.
+            mem::swap(buf, &mut line);
+            Poll::Ready(ret)
+        }
+    }
+}
+
 #[cfg(feature = "async")]
 #[cfg_attr(docsrs, doc(cfg(feature = "async")))]
 impl<R: AsyncBufRead + Unpin> AsyncRead for ArmoredReader<R> {
@@ -891,30 +943,20 @@ impl<R: AsyncBufRead + Unpin> AsyncRead for ArmoredReader<R> {
 
                     // Read the next line
                     {
+                        // Emulates `AsyncBufReadExt::read_line`.
                         let mut this = self.as_mut().project();
-                        let available = loop {
-                            let buf = ready!(this.inner.as_mut().poll_fill_buf(cx))?;
-                            if buf.contains(&b'\n') {
-                                break buf;
-                            }
-                        };
-                        let pos = available
-                            .iter()
-                            .position(|c| *c == b'\n')
-                            .expect("contains LF byte")
-                            + 1;
-
-                        this.line_buf
-                            .push_str(std::str::from_utf8(&available[..pos]).map_err(|_| {
-                                io::Error::new(
-                                    io::ErrorKind::InvalidData,
-                                    ArmoredReadError::InvalidUtf8,
-                                )
-                            })?);
-
-                        this.inner.as_mut().consume(pos);
-                        self.count_reader_bytes(pos);
+                        let buf: &mut String = &mut this.line_buf;
+                        let mut bytes = mem::take(buf).into_bytes();
+                        let mut read = 0;
+                        ready!(read_line_internal(
+                            this.inner.as_mut(),
+                            cx,
+                            buf,
+                            &mut bytes,
+                            &mut read,
+                        ))
                     }
+                    .map(|read| self.count_reader_bytes(read))?;
 
                     // Parse the line into bytes.
                     let read = if self.parse_armor_line()? {

age/tests/testkit.rs

@@ -156,7 +156,7 @@ fn testkit(filename: &str) {
 #[test_case("armor_empty_line_end")]
 #[test_case("armor_eol_between_padding")]
 #[test_case("armor_full_last_line")]
-// #[test_case("armor_garbage_encoded")]
+#[test_case("armor_garbage_encoded")]
 #[test_case("armor_garbage_leading")]
 #[test_case("armor_garbage_trailing")]
 #[test_case("armor_header_crlf")]
@@ -165,8 +165,8 @@ fn testkit(filename: &str) {
 #[test_case("armor_invalid_character_payload")]
 #[test_case("armor_long_line")]
 #[test_case("armor_lowercase")]
-// #[test_case("armor_no_end_line")]
-// #[test_case("armor_no_eol")]
+#[test_case("armor_no_end_line")]
+#[test_case("armor_no_eol")]
 #[test_case("armor_no_match")]
 #[test_case("armor_no_padding")]
 #[test_case("armor_not_canonical")]