ruuda / claxon Goto Github PK
View Code? Open in Web Editor NEWA FLAC decoder in Rust
License: Apache License 2.0
A FLAC decoder in Rust
License: Apache License 2.0
Hello ๐ฆ ,
we (Rust group @sslab-gatech) found a memory-safety/soundness issue in this crate while scanning Rust code on crates.io for potential vulnerabilities.
metadata::read_vorbis_comment_block()
methodLines 432 to 438 in 2f05385
Lines 473 to 475 in 2f05385
metadata::read_application_block()
methodLines 544 to 546 in 2f05385
Methods metadata::read_vorbis_comment_block()
& metadata::read_application_block()
create an uninitialized buffer and passes it to user-provided ReadBytes
implementation. This is unsound, because it allows safe Rust code to exhibit an undefined behavior (read from uninitialized memory).
This part from the Read
trait documentation explains the issue:
It is your responsibility to make sure that
buf
is initialized before callingread
. Calling read with an uninitializedbuf
(of the kind one obtains viaMaybeUninit<T>
) is not safe, and can lead to undefined behavior.
It is safe to zero-initialize the newly allocated u8
buffer before read_into()
, in order to prevent user-provided Read
from accessing old contents of the newly allocated heap memory.
Also, there are two nightly features for handling such cases.
Thank you for checking out this issue ๐
It would be nice to refactor ensure_buffer_len()
into safe code so that bugs such as #10 are statically ruled out by the compiler, without sacrificing performance.
This is interesting from the API design and best practices perspective. Writing data into a preallocated buffer is a very common thing to do in systems programming, so the compiler and stdlib should support a safe and idiomatic way to do it. This may be a good case study for unsafe code guidelines WG and/or the emerging security working group.
Some ideas to get the ball rolling:
vec![0, len]
that the stdlib optimizes into as cheap memory zeroing as possible. Sadly this is still going to involve some overhead, unless we convince the optimizer that the entire buffer is always overwritten.Flac doesn't have to be inside its own custom container, it can also be encapsulated inside ogg or inside the mp4 container. It would be cool if this crate supported those use cases, maybe through optional cargo features.
My lewton crate is organized in a very generic fashion: its main function only takes a byte slice and treats that as one packet, regardless of the actual container format. The entire crate knows nothing about the container format, except for the inside_ogg
module. Having such a setup for the claxon crate would be really cool.
I'm working on a thing that splits FLAC files with embedded CUE sheets into separate tracks, and would love to use this crate to do that: It's got most of the building blocks (a nice StreamInfo representation, good metadata support, a fine data model!), but it doesn't look like I can make use of the library getting decoded samples back.
Would this kind of usage mode be a useful contribution?
I have a FLAC file encoded with the reference implementation (libFLAC) which has a frame (probably the last) with block size 8. The file has minimum and maximum block size of 4096 (fixed block size stream). All other frames have block size 4096.
When I parse it with claxon, though, I get the error invalid block size, must be at least 16
.
If I understand correctly the specification (Note 3), this is allowed, as the last frame of a fixed block size stream can be shorter than the minimum block size defined in STREAMINFO block and thus shorter than 16 as well.
Sorry for bothering you all this way, I really hope you don't mind.
To implement FLAC decoding in Claxon, you might have used the FLAC format specification on the FLAC website. If so, you probably needed to dive into some actual code (libFLAC or ffmpeg) to understand the finer points. An IETF working group has tried to improve this document, trying to include those finer points. In fact, I know @ruuda contributed to this a few years ago. However, all but a few lines have changed since and it is on the verge of being turned in for final checks and publication as an RFC. A so-called working group last call has been issued, which lasts 2 weeks.
As you've had first-hand experience implementing FLAC from scratch, you probably know best what implementation details needed extra attention. It would be very valuable to have your feedback on the FLAC format for this document.
I hope you can provide some insights.
The latest draft can be found here https://datatracker.ietf.org/doc/draft-ietf-cellar-flac/
I got a few files containing tailing empty vorbis comment. I am aware that no "specification" explicitly allows this, but all applications (vlc, rythmbox, totem, my old mp3-player, etc.) that i tested, are able to correctly process these files.
So my suggestion would be to just ignore empty comments.
Hello,
Is it possible to decode "flac tag" (or vorbis comments) with your library ?
It seems that it is exposed in the library through MetadataBlock, but I am not sure.
Thanks !
When I wanted to update audrey to claxon version 0.3, I saw that claxon::FlacSamples
now depends on only one type, which is generic on a Trait. Now there are a few problems:
So what about a facade instead? If there was a public facade/wrapper for FrameReader which is generic on the io::Read trait, my problems would be solved.
Maybe there are further issues that can occur, not sure but if you pushed an updated version to github I will try it out :).
Hello!
First thank you for this library. I've found it very approachable as a Rust beginner :)
I've written some code that reads a Flac file into memory as a Vec<f32>
based off one of the given examples.
const CHANNELS: usize = 2;
type Sample = f32;
type Frame = [Sample; CHANNELS];
type Media = Vec<Frame>;
pub fn read_flac(path: String) -> Media {
let mut reader = claxon::FlacReader::open(path).expect("Unable to open FLAC file");
let mut samples = reader.samples();
let capacity = (audio_engine::SAMPLE_HZ * 60.0 * 10.0) as usize;
let mut frames = Vec::with_capacity(capacity);
while let (Some(Ok(l)), Some(Ok(r))) = (samples.next(), samples.next()) {
frames.push([l as Sample, r as Sample]);
}
frames.shrink_to_fit();
frames
}
fn main() {
println!("About to read file");
let _media = media::read_flac("../media/music.flac".to_string());
println!("Done");
}
When I run this it takes longer than I would expect.
louis ~/projects/decksterity/rust [master *] $ time cargo run
Finished dev [unoptimized + debuginfo] target(s) in 0.0 secs
Running `target/debug/decksterity`
about to read file
done
real 0m21.641s
user 0m21.604s
sys 0m0.028s
Here the given input file was 26MiB and it took 22 seconds to read it.
This is slower than I would expect, other programs (not written by me) on the same machine seem to be able to read the entire file in less than a second.
Have I made a mistake in my implementation here?
Thanks,
Louis
On certain malformed inputs contents of uninitialized memory are written to the decoded audio.
This is a security vulnerability. Examples of similar vulnerabilities in C code and discussion of the potential impact can be found here. I have also discussed a similar bug in Rust png
crate in my Auditing popular crates post.
This issue has been discovered using differential fuzzing with afl-fuzz, similar to the C vulnerabilities linked above. I shall relay further details on the issue to the maintainer privately by email.
The trivial hotfix is replacing the following line
Line 618 in b4a89e4
with buffer.resize(new_len, 0);
, but is likely to degrade performance. There are some tricks that can reduce the cost of zeroing the memory, but this approach is bound to be slower than using uninitialized memory.
However, these kinds of issues can be ruled out systemically by passing a reference to the vector to subframe::decode
instead of a slice with uninitialized memory and using something like Write trait or extend_from_slice()
to write to the vector safely without zeroing the memory first.
Once a fix is published, the issue should be added to the Rust security advisory database.
Hi!
I'm trying to use claxon
in an embedded environment and would like to reuse the decode buffer, but I'm not figuring out how to do this when I might have multiple frames to decode in a single buffer:
impl Decode for FlacDecoder {
fn decode_sample(&mut self, buf: &[u8], out: &mut [i16]) -> Result<usize, anyhow::Error> {
let mut fr = FrameReader::new(std::io::Cursor::new(buf));
let mut c = 0;
let mut v = std::mem::take(&mut self.dec_buf);
loop {
if let Ok(Some(block)) = fr.read_next_or_eof(v) {
for (a, b) in block.stereo_samples() {
out[c + 0] = a as i16;
out[c + 1] = b as i16;
c += 2;
}
v = block.into_buffer();
} else {
break;
}
}
// self.dec_buf = v;
// can't do this, `v` was moved into `read_next_or_eof`
// and both the `Err` and `Ok(None)` cases would lose the buffer
Ok(c)
}
}
I've made a branch with an API change which allows passing a mutable reference, but this breaks the FlacSamples
and FlacIntoSamples
iterators
Vorbis comment headers appear in quite many places, in vorbis, opus and in flac. Even though parsing them is only very little code, I think one could create a crate to do the parsing. What do you think? Would you use it? If yes, I'll extract the parsing code from my lewton crate :).
I'm thinking of contributing this crate (specifically, add seek feature!). Do you have MSRV for this crate in mind? Also, do I have to stick to Edition 2015 instead of the latest 2018?
I have a FLAC file with this frame: FF F8 C9 18 E0 B2 95 8E 01 80 01 00 00 03 28 92
followed by this frame: FF F8 C9 18 E0 B2 96 87 01 80 01 00 00 03 11 08
.
Using flac --analyze I get the following for this 2 frames:
frame=3221 offset=26178049 bits=128 blocksize=4096 sample_rate=44100 channels=2 channel_assignment=INDEPENDENT
subframe=0 wasted_bits=1 type=CONSTANT value=1
subframe=1 wasted_bits=0 type=CONSTANT value=3
frame=3222 offset=26178065 bits=128 blocksize=4096 sample_rate=44100 channels=2 channel_assignment=INDEPENDENT
subframe=0 wasted_bits=1 type=CONSTANT value=1
subframe=1 wasted_bits=0 type=CONSTANT value=3
But with claxon I get the following:
Frame Number: FrameNumber(3221)
Block Size: 4096
Sample Rate: Some(44100)
Channel Assignment: Independent(2)
Bits per Sample: Some(16)
CRC8: 8e (Expected: 8e)
Subframe Type: Constant
Wasted Bits: 1
Constant Value: 2
Subframe Type: Constant
Wasted Bits: 0
Constant Value: 6
CRC16: 9200 (Expected: 92ff)
As you can see it parses wrong the two subframes, reading past the frame (it reads the value 92 FF
for the frame CRC instead of 28 92
as it should).
Checking the reference implementation I found that the wasted bits are subtracted from the frames' bits per sample (source), something that claxon doesn't do and that's why it reads more bits than it should, resulting in frame CRC mismatch.
Adding this snippet
let mut bps = bps;
if header.wasted_bits_per_sample > 0 {
bps -= header.wasted_bits_per_sample;
}
here, the file is decoded fine, although there should be a check that the wasted bits are not more than the bits per sample.
Latest version of claxon requires hound 0.4.0, which does not compile.
I get the following errors:
Compiling hound v0.4.0
path/.cargo/registry/src/github.com-88ac128001ac3a9a/hound-0.4.0/src/lib.rs:84:5: 84:81 error: the trait `core::marker::Sized` is not implemented for the type `Self` [E0277]
path/.cargo/registry/src/github.com-88ac128001ac3a9a/hound-0.4.0/src/lib.rs:84 fn read<R: io::Read>(reader: &mut R, bytes: u16, bits: u16) -> Result<Self>;
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
path/.cargo/registry/src/github.com-88ac128001ac3a9a/hound-0.4.0/src/lib.rs:84:5: 84:81 help: run `rustc --explain E0277` to see a detailed explanation
path/.cargo/registry/src/github.com-88ac128001ac3a9a/hound-0.4.0/src/lib.rs:84:5: 84:81 note: `Self` does not have a constant size known at compile-time
path/.cargo/registry/src/github.com-88ac128001ac3a9a/hound-0.4.0/src/lib.rs:84:5: 84:81 note: required by `core::result::Result`
error: aborting due to previous error
Could not compile `hound`.
I'm using rust 1.7.0
I want to play some audio files in rust, so I used the rodio crate. Rodio uses claxon for decoding FLAC files, and some files didn't work with rodio.
When I tried the decode_simple.rs example's code from this repo, I got this error: thread 'main' panicked at 'failed to decode FLAC stream: Unsupported("LPC order > 12 is not supported")', src/libcore/result.rs:999:5
. Here's a file that doesn't work. (Edit by @ruuda: link to private file removed.)
There is a fairly complicated workaround I came up with, here's the code. This method is, however, pretty slow. Can this feature be implemented in claxon?
I'm planning to add seeking feature to this library, as I mentioned in #28 . This issue has been opened for tracking the design plan for this new feature.
First, we are going to add new constructors new_seekable
and new_seekable_ext
in impl <R: Read + Seek> FlacReader {
. The purpose of creating a new struct is that we have to do an additional process right after reading all the metadata blocks: save the position of the reader. This enables the reader to determine the range for binary searching for every seek. This position is going to be stored in a new field of FlacReader
, audio_block_start_position: Option<u64>
.
Next, we are going to add new function seek
to FrameReader
and FlacSamples
, both of which takes the sample number (0-indexed).
FrameReader::seek
seeks the reader to the beginning of the desired frame. Here, the "desired frame" is the last frame whose frame number is less than or equal to the given sample number (in case of fixed block size stream, it is automatically converted appropriately). Therefore, the next frame (this is equivalent to claxon::frame::Block
, right?) yielded from FrameReader
will include the desired sample (if the given sample number was less than the range of the length of the audio stream).
std::io::SeekFrom::End(0)
. However, if possible, this function uses SEEKTABLE
metadata to narrow down the range, and thus speed up the seeking process.
claxon::metadata::SeekTable
does not expose any API to access the data. Shall I add some as well?BTreeMap
rather than Vec
.FRAME_HEADER
, by matching both sync code and 8-bit CRC provided at last. Calculating CRC-16 for entire frame makes it even more robust, but I think it is excessive work, and there are very low possibility that CRC-8 accidentally matches.FlacSamples::seek
seeks the reader to the desired sample. That is, the next sample yielded from the iterator will precisely has the sample index that equal to the provided argument. Internally, it calls FrameReader::seek
and then discards the first few frames in the first block obtained.audio_block_start_position
will be stored as Option<u64>
, as it may be vacant for non-seekable reader while filled for seekable one. But this is independent of whether the reader is actually seekable or not. That is, one can use the original constructor FrameReader::new
while providing seekable reader, and then call seek
function. So, the field may be None
for the first time seek
function is called, in which case we have to go back to the beginning of the stream and actually fill it with appropriate value. This may not be a big problem, but something that can be avoided by "type puzzle": For example, we can define a marker trait IsSeekable
, create two marker structs Seekable
and NonSeekable
, accept S: IsSeekable
as additional type parameter for FlacReader
, FrameReader
and FlacSamples
, and define associated type IsSeekable::AudioBlockStartPosition
which is set to ()
for NonSeekable
and u64
for Seekable
. Whew. Personally I love this kind of type puzzle but I doubt it's suitable for public library. Alternatively, we can define seekable variants for FlacReader
, FrameReader
and FlacSamples
, which may make API less understandable and make code confusing.Thanks for reading this lengthy draft! Feel free to make questions or oppositions. I'll get started in a few days anyway.
Given that Claxon is used by Rodio as it's FLAC support, I was wondering if ya'll have considered adding encoding support?
My use case is effectively that I want to convert a wav file to FLAC, but it doesn't seem that there is any Rust FLAC encoders out there yet.
Hi,
Excellent library!
It's defined over io::Read
but I need to use it on a WASM target that will receive data from javascript Promises. What would be the best way to use the library in an environment where future bytes may not exist yet?
Would returning io::ErrorKind::WouldBlock
from io::Read
make the reads for samples retryable?
A declarative, efficient, and flexible JavaScript library for building user interfaces.
๐ Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
An Open Source Machine Learning Framework for Everyone
The Web framework for perfectionists with deadlines.
A PHP framework for web artisans
Bring data to life with SVG, Canvas and HTML. ๐๐๐
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
Some thing interesting about web. New door for the world.
A server is a program made to process requests and deliver data to clients.
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
Some thing interesting about visualization, use data art
Some thing interesting about game, make everyone happy.
We are working to build community through open source technology. NB: members must have two-factor auth.
Open source projects and samples from Microsoft.
Google โค๏ธ Open Source for everyone.
Alibaba Open Source for everyone
Data-Driven Documents codes.
China tencent open source team.