From a06fd8d54e61a96cf0873e69a86086e65b9112a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Gonz=C3=A1lez?= Date: Sat, 12 Oct 2024 14:45:54 +0200 Subject: [PATCH 1/6] Fix Cargo warning on latest Rust versions about custom cfg flags --- Cargo.toml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Cargo.toml b/Cargo.toml index e34242c..6caa7fd 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -34,3 +34,6 @@ async = ["futures-core", "futures-io", "tokio", "tokio-util", "bytes", "pin-proj [package.metadata.docs.rs] all-features = true rustdoc-args = ["--cfg", "docsrs"] + +[lints.rust] +unexpected_cfgs = { level = "warn", check-cfg = ['cfg(fuzzing)'] } From a1a74a58f4ff787233d877bfe60537f6b687abb0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Gonz=C3=A1lez?= Date: Sat, 12 Oct 2024 14:23:33 +0200 Subject: [PATCH 2/6] Make page CRC verification toggleable by end-users These changes make the Ogg page CRC verification check toggleable at runtime by end-users via a new `PageParsingOptions` struct that may be passed at reader/parser construction time. I have paid special attention to making the new feature both backwards and forwards compatible, which led me to taking the following design choices: - No signature of any existing public method was modified. People looking into changing parsing behavior via the new `PageParsingOptions` struct must use the new `*_with_parse_opts` methods. - Marking `PageParsingOptions` as `#[non_exhaustive]`, so that we may add new public fields to it in the future without a semver-breaking change. Users must always default-initialize this struct. It only derives `Clone` too, in case we ever need to make it hold non-`Copy` types. - Shared ownership of the `PageParsingOptions` between different reader structs is achieved through `Arc`s, which ensures that no struct stops being `Send` or `Sync` with a negligble performance impact. The `fuzzing` cfg flag is still honored after these changes by using it to set a default value for the new `verify_hash` page parsing option accordingly. --- src/lib.rs | 2 +- src/reading.rs | 124 ++++++++++++++++++++++++++++++++++++++----------- 2 files changed, 97 insertions(+), 29 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 146f4a4..081ae2a 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -40,7 +40,7 @@ pub mod reading; pub mod writing; pub use crate::writing::{PacketWriter, PacketWriteEndInfo}; -pub use crate::reading::{PacketReader, OggReadError}; +pub use crate::reading::{PacketReader, OggReadError, PageParsingOptions}; /** Ogg packet representation. diff --git a/src/reading.rs b/src/reading.rs index 7bdf789..cd129f6 100644 --- a/src/reading.rs +++ b/src/reading.rs @@ -21,6 +21,7 @@ use std::mem::replace; use crate::crc::vorbis_crc32_update; use crate::Packet; use std::io::Seek; +use std::sync::Arc; /// Error that can be raised when decoding an Ogg transport. #[derive(Debug)] @@ -157,6 +158,28 @@ impl OggPage { } } +/// A set of options that control details on how a `PageParser` parses OGG pages. +#[derive(Debug, Clone)] +#[non_exhaustive] // Allow for non-breaking field expansion: https://doc.rust-lang.org/cargo/reference/semver.html#struct-add-public-field-when-no-private +pub struct PageParsingOptions { + /// Whether the CRC checksum for the parsed pages will be verified to match + /// the page and packet data. Verifying it is the default behavior, as + /// a hash mismatch usually signals stream corruption, but specialized + /// applications may wish to deal with the contained data anyway. + pub verify_checksum :bool, +} + +impl Default for PageParsingOptions { + fn default() -> Self { + Self { + // Do not verify checksum by default when the decoder is being fuzzed. + // This makes it easier for random input from fuzzers to reach decoding code that's + // actually interesting, instead of being rejected early due to checksum mismatch. + verify_checksum: !cfg!(fuzzing), + } + } +} + /** Helper struct for parsing pages @@ -178,6 +201,8 @@ pub struct PageParser { /// after segments have been parsed, this contains the segments buffer, /// after the packet data have been read, this contains the packets buffer. segments_or_packets_buf :Vec, + + parse_opts :Arc, } impl PageParser { @@ -193,6 +218,23 @@ impl PageParser { /// You should allocate and fill such an array, in order to pass it to the `parse_segments` /// function. pub fn new(header_buf :[u8; 27]) -> Result<(PageParser, usize), OggReadError> { + Self::new_with_parse_opts(header_buf, PageParsingOptions::default()) + } + + /// Creates a new Page parser with the specified `parse_opts` + /// + /// The `header_buf` param contains the first 27 bytes of a new OGG page. + /// Determining when one begins is your responsibility. Usually they + /// begin directly after the end of a previous OGG page, but + /// after you've performed a seek you might end up within the middle of a page + /// and need to recapture. + /// + /// `parse_opts` controls details on how the OGG page will be parsed. + /// + /// Returns a page parser, and the requested size of the segments array. + /// You should allocate and fill such an array, in order to pass it to the `parse_segments` + /// function. + pub fn new_with_parse_opts(header_buf :[u8; 27], parse_opts :impl Into>) -> Result<(PageParser, usize), OggReadError> { let mut header_rdr = Cursor::new(header_buf); header_rdr.set_position(4); let stream_structure_version = tri!(header_rdr.read_u8()); @@ -221,6 +263,7 @@ impl PageParser { header_buf, packet_count : 0, segments_or_packets_buf :Vec::new(), + parse_opts: parse_opts.into(), }, // Number of page segments header_rdr.read_u8().unwrap() as usize @@ -271,31 +314,28 @@ impl PageParser { page_siz as usize } - /// Parses the packets data and verifies the checksum. + /// Parses the packets data and verifies the checksum if appropriate. /// /// Returns an `OggPage` to be used by later code. pub fn parse_packet_data(mut self, packet_data :Vec) -> Result { - // Now to hash calculation. - // 1. Clear the header buffer - self.header_buf[22] = 0; - self.header_buf[23] = 0; - self.header_buf[24] = 0; - self.header_buf[25] = 0; - - // 2. Calculate the hash - let mut hash_calculated :u32; - hash_calculated = vorbis_crc32_update(0, &self.header_buf); - hash_calculated = vorbis_crc32_update(hash_calculated, - &self.segments_or_packets_buf); - hash_calculated = vorbis_crc32_update(hash_calculated, &packet_data); - - // 3. Compare to the extracted one - if self.checksum != hash_calculated { - // Do not verify checksum when the decoder is being fuzzed. - // This allows random input from fuzzers reach decoding code that's actually interesting, - // instead of being rejected early due to checksum mismatch. - if !cfg!(fuzzing) { + if self.parse_opts.verify_checksum { + // Now to hash calculation. + // 1. Clear the header buffer + self.header_buf[22] = 0; + self.header_buf[23] = 0; + self.header_buf[24] = 0; + self.header_buf[25] = 0; + + // 2. Calculate the hash + let mut hash_calculated :u32; + hash_calculated = vorbis_crc32_update(0, &self.header_buf); + hash_calculated = vorbis_crc32_update(hash_calculated, + &self.segments_or_packets_buf); + hash_calculated = vorbis_crc32_update(hash_calculated, &packet_data); + + // 3. Compare to the extracted one + if self.checksum != hash_calculated { tri!(Err(OggReadError::HashMismatch(self.checksum, hash_calculated))); } } @@ -721,6 +761,7 @@ and look into the async module. */ pub struct PacketReader { rdr :T, + pg_parse_opts :Arc, base_pck_rdr :BasePacketReader, @@ -730,7 +771,11 @@ pub struct PacketReader { impl PacketReader { /// Constructs a new `PacketReader` with a given `Read`. pub fn new(rdr :T) -> PacketReader { - PacketReader { rdr, base_pck_rdr : BasePacketReader::new(), read_some_pg : false } + Self::new_with_page_parse_opts(rdr, PageParsingOptions::default()) + } + /// Constructs a new `PacketReader` with a given `Read` and OGG page parsing options. + pub fn new_with_page_parse_opts(rdr :T, pg_parse_opts : impl Into>) -> PacketReader { + PacketReader { rdr, pg_parse_opts: pg_parse_opts.into(), base_pck_rdr : BasePacketReader::new(), read_some_pg : false } } /// Returns the wrapped reader, consuming the `PacketReader`. pub fn into_inner(self) -> T { @@ -820,14 +865,14 @@ impl PacketReader { None if self.read_some_pg => return Ok(None), None => return Err(OggReadError::NoCapturePatternFound) }; - let (mut pg_prs, page_segments) = tri!(PageParser::new(header_buf)); + let (mut pg_prs, page_segments) = tri!(PageParser::new_with_parse_opts(header_buf, Arc::clone(&self.pg_parse_opts))); let mut segments_buf = vec![0; page_segments]; // TODO fix this, we initialize memory for NOTHING!!! Out of some reason, this is seen as "unsafe" by rustc. tri!(self.rdr.read_exact(&mut segments_buf)); let page_siz = pg_prs.parse_segments(segments_buf); - let mut packet_data = vec![0; page_siz as usize]; + let mut packet_data = vec![0; page_siz]; tri!(self.rdr.read_exact(&mut packet_data)); Ok(Some(tri!(pg_prs.parse_packet_data(packet_data)))) @@ -1088,12 +1133,14 @@ pub mod async_api { */ struct PageDecoder { state : PageDecodeState, + parse_opts : Arc, } impl PageDecoder { - fn new() -> Self { + fn new(parse_opts : impl Into>) -> Self { PageDecoder { state : PageDecodeState::Head, + parse_opts : parse_opts.into(), } } } @@ -1119,7 +1166,7 @@ pub mod async_api { // TODO once we have const generics, the copy below can be done // much nicer, maybe with a new into_array fn on Vec's hdr_buf.copy_from_slice(&consumed_buf); - let tup = tri!(PageParser::new(hdr_buf)); + let tup = tri!(PageParser::new_with_parse_opts(hdr_buf, Arc::clone(&self.parse_opts))); Segments(tup.0, tup.1) }, Segments(mut pg_prs, _) => { @@ -1162,9 +1209,18 @@ pub mod async_api { /// This is the recommended constructor when using the Tokio runtime /// types. pub fn new(inner :T) -> Self { + Self::new_with_page_parse_opts(inner, PageParsingOptions::default()) + } + + /// Wraps the specified Tokio runtime `AsyncRead` into an Ogg packet + /// reader, using the specified options for parsing Ogg pages. + /// + /// This is the recommended constructor when using the Tokio runtime + /// types. + pub fn new_with_page_parse_opts(inner :T, pg_parse_opts :impl Into>) -> Self { PacketReader { base_pck_rdr : BasePacketReader::new(), - pg_rd : FramedRead::new(inner, PageDecoder::new()), + pg_rd : FramedRead::new(inner, PageDecoder::new(pg_parse_opts)), } } } @@ -1179,7 +1235,19 @@ pub mod async_api { /// from other runtimes, and implementing a Tokio `AsyncRead` /// compatibility layer oneself is not desired. pub fn new_compat(inner :T) -> Self { - Self::new(inner.compat()) + Self::new_compat_with_page_parse_opts(inner.compat(), PageParsingOptions::default()) + } + + /// Wraps the specified futures_io `AsyncRead` into an Ogg packet + /// reader, using the specified options for parsing Ogg pages. + /// + /// This crate uses Tokio internally, so a wrapper that may have + /// some performance cost will be used. Therefore, this constructor + /// is to be used only when dealing with `AsyncRead` implementations + /// from other runtimes, and implementing a Tokio `AsyncRead` + /// compatibility layer oneself is not desired. + pub fn new_compat_with_page_parse_opts(inner :T, pg_parse_opts :impl Into>) -> Self { + Self::new_with_page_parse_opts(inner.compat(), pg_parse_opts) } } From 04cbc3835240ea0d23a1c10d9a41b6e1c2074e4c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Gonz=C3=A1lez?= Date: Sat, 12 Oct 2024 15:52:59 +0200 Subject: [PATCH 3/6] Fix build with `async` feature --- src/reading.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/reading.rs b/src/reading.rs index cd129f6..ea8fca5 100644 --- a/src/reading.rs +++ b/src/reading.rs @@ -1235,7 +1235,7 @@ pub mod async_api { /// from other runtimes, and implementing a Tokio `AsyncRead` /// compatibility layer oneself is not desired. pub fn new_compat(inner :T) -> Self { - Self::new_compat_with_page_parse_opts(inner.compat(), PageParsingOptions::default()) + Self::new_compat_with_page_parse_opts(inner, PageParsingOptions::default()) } /// Wraps the specified futures_io `AsyncRead` into an Ogg packet From a859baa832c2d38c948329c9274985ad9183dcc0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Gonz=C3=A1lez?= Date: Sat, 12 Oct 2024 15:58:19 +0200 Subject: [PATCH 4/6] Attempt to fix MSRV build --- .github/workflows/ogg.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ogg.yml b/.github/workflows/ogg.yml index 445a0b3..a6b68da 100644 --- a/.github/workflows/ogg.yml +++ b/.github/workflows/ogg.yml @@ -21,7 +21,9 @@ jobs: override: true - name: Downgrade dependencies to comply with MSRV if: matrix.toolchain == '1.56.1' - run: cargo update -p tokio --precise 1.29.1 + run: | + cargo update -p tokio --precise 1.29.1 + cargo update -p ppv-lite86 --precise 0.2.17 - name: Run no-default-features builds if: matrix.toolchain != '1.56.1' run: | From 8fe0cfbbe20449be142b5b3ecdb85fcfb2edec0b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Gonz=C3=A1lez?= Date: Sat, 12 Oct 2024 16:09:18 +0200 Subject: [PATCH 5/6] Bump MSRV to 1.61 We required it anyway due to recent changes. --- .github/workflows/ogg.yml | 13 ++++--------- Cargo.toml | 2 +- README.md | 2 +- 3 files changed, 6 insertions(+), 11 deletions(-) diff --git a/.github/workflows/ogg.yml b/.github/workflows/ogg.yml index a6b68da..7d518c8 100644 --- a/.github/workflows/ogg.yml +++ b/.github/workflows/ogg.yml @@ -8,7 +8,7 @@ jobs: strategy: matrix: os: [macOS-latest, ubuntu-latest, windows-latest] - toolchain: [stable, beta, 1.56.1] + toolchain: [stable, beta, 1.61] runs-on: ${{ matrix.os }} @@ -19,25 +19,20 @@ jobs: with: toolchain: ${{ matrix.toolchain }} override: true - - name: Downgrade dependencies to comply with MSRV - if: matrix.toolchain == '1.56.1' - run: | - cargo update -p tokio --precise 1.29.1 - cargo update -p ppv-lite86 --precise 0.2.17 - name: Run no-default-features builds - if: matrix.toolchain != '1.56.1' + if: matrix.toolchain != '1.61' run: | cargo test --verbose --no-default-features cargo doc --verbose --no-default-features - name: Run no-default-features builds (forbidding warnings) - if: matrix.toolchain == '1.56.1' + if: matrix.toolchain == '1.61' env: RUSTFLAGS: -D warnings run: | cargo test --verbose --no-default-features cargo doc --verbose --no-default-features - name: Run all-features builds - if: matrix.toolchain != '1.56.1' + if: matrix.toolchain != '1.61' run: | cargo test --verbose --all-features cargo doc --verbose --all-features diff --git a/Cargo.toml b/Cargo.toml index 6caa7fd..27cd680 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -9,7 +9,7 @@ keywords = ["ogg", "decoder", "encoder", "xiph"] documentation = "https://docs.rs/ogg/0.8.0" repository = "https://github.com/RustAudio/ogg" readme = "README.md" -rust-version = "1.56.0" +rust-version = "1.61.0" [lib] name = "ogg" diff --git a/README.md b/README.md index c6081dd..9584da1 100644 --- a/README.md +++ b/README.md @@ -2,7 +2,7 @@ An Ogg decoder and encoder. Implements the [xiph.org Ogg spec](https://www.xiph.org/vorbis/doc/framing.html) in pure Rust. -If the `async` feature is disabled, Version 1.56.1 of Rust is the minimum supported one. +If the `async` feature is disabled, Version 1.61 of Rust is the minimum supported one. Note: `.ogg` files are vorbis encoded audio files embedded into an Ogg transport stream. There is no extra support for vorbis codec decoding or encoding in this crate, From 230d8db74f440d2ed19ccd567559d25d7953135d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Gonz=C3=A1lez?= Date: Sat, 12 Oct 2024 16:13:44 +0200 Subject: [PATCH 6/6] Further CI MSRV check fixes --- .github/workflows/ogg.yml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.github/workflows/ogg.yml b/.github/workflows/ogg.yml index 7d518c8..fc989ad 100644 --- a/.github/workflows/ogg.yml +++ b/.github/workflows/ogg.yml @@ -24,6 +24,9 @@ jobs: run: | cargo test --verbose --no-default-features cargo doc --verbose --no-default-features + - name: Downgrade dependencies to comply with MSRV + if: matrix.toolchain == '1.61' + run: cargo update -p tokio --precise 1.29.1 - name: Run no-default-features builds (forbidding warnings) if: matrix.toolchain == '1.61' env: