Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle integer overflows and other stuff #1619

Merged
merged 10 commits into from
Sep 3, 2019
Merged

Conversation

YaLTeR
Copy link
Collaborator

@YaLTeR YaLTeR commented Aug 30, 2019

I fuzzed things. Things broke.

Fixes #1616.

This changes the type of rdo_lookahead_frames in EncoderConfig from u64 to usize. API change; merge this before 0.1 if possible?

@YaLTeR YaLTeR requested review from lu-zero and tdaede August 30, 2019 23:54
src/api/mod.rs Outdated Show resolved Hide resolved
src/api/mod.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@lu-zero lu-zero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can try to prepare a little more structured errors so we could hint what's wrong and what's the correct value-range. Ideally we could move all the validation in a function like:

Config::validate() ->Result<(), ConfigError>

and call it inside the Context factory.

src/api/mod.rs Outdated Show resolved Hide resolved
src/api/mod.rs Outdated Show resolved Hide resolved
@YaLTeR
Copy link
Collaborator Author

YaLTeR commented Sep 3, 2019

Fixed, rebased, etc.

@YaLTeR YaLTeR requested a review from lu-zero September 3, 2019 18:25
Copy link
Collaborator

@lu-zero lu-zero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a good step in the right direction, some refactoring and considerations may happen later.

@@ -160,11 +160,12 @@ pub struct Sequence {
}

impl Sequence {
pub fn new(config: &EncoderConfig) -> Sequence {
pub fn new(config: &EncoderConfig) -> Option<Sequence> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd keep the assert and move the check (using ilog maybe) where the configuration is validated. The sequence is an internal detail and should never fail no matter the input.

@coveralls
Copy link
Collaborator

Coverage Status

Coverage decreased (-0.3%) to 69.579% when pulling 6399bb2 on YaLTeR:fix-things into b9975f8 on xiph:master.

@YaLTeR YaLTeR merged commit d512dc8 into xiph:master Sep 3, 2019
@YaLTeR YaLTeR mentioned this pull request Sep 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flushing without any frames results in NeedMoreData
4 participants