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

Panic on invalid input to jpeg_decoder::Decoder::decode #277

Open
MinghuaWang opened this issue Feb 6, 2024 · 2 comments
Open

Panic on invalid input to jpeg_decoder::Decoder::decode #277

MinghuaWang opened this issue Feb 6, 2024 · 2 comments

Comments

@MinghuaWang
Copy link

Describe the bug

Panic could be triggered when passing jpeg_decoder::Decoder::decode with invalid input. Panic info is shown below:
thread 'main' panicked at /home/test/.cargo/registry/src/mirrors.ustc.edu.cn-61ef6e0cd06fb9b8/jpeg-decoder-0.3.1/src/decoder/lossless.rs:112:40: attempt to subtract with overflow

Full stack backtrace:
0: rust_begin_unwind
at /rustc/07688726805d5db0a4bca445a6651d09708041ea/library/std/src/panicking.rs:617:5
1: core::panicking::panic_fmt
at /rustc/07688726805d5db0a4bca445a6651d09708041ea/library/core/src/panicking.rs:67:14
2: core::panicking::panic
at /rustc/07688726805d5db0a4bca445a6651d09708041ea/library/core/src/panicking.rs:117:5
3: jpeg_decoder::decoder::lossless::<impl jpeg_decoder::decoder::Decoder>::decode_scan_lossless
at /home/test/.cargo/registry/src/mirrors.ustc.edu.cn-61ef6e0cd06fb9b8/jpeg-decoder-0.3.1/src/decoder/lossless.rs:112:40
4: jpeg_decoder::decoder::Decoder::decode_internal
at /home/test/.cargo/registry/src/mirrors.ustc.edu.cn-61ef6e0cd06fb9b8/jpeg-decoder-0.3.1/src/decoder.rs:415:46
5: jpeg_decoder::decoder::Decoder::decode::{{closure}}
at /home/test/.cargo/registry/src/mirrors.ustc.edu.cn-61ef6e0cd06fb9b8/jpeg-decoder-0.3.1/src/decoder.rs:294:36
6: jpeg_decoder::worker::WorkerScope::with
at /home/test/.cargo/registry/src/mirrors.ustc.edu.cn-61ef6e0cd06fb9b8/jpeg-decoder-0.3.1/src/worker/mod.rs:61:9
7: jpeg_decoder::decoder::Decoder::decode
at /home/test/.cargo/registry/src/mirrors.ustc.edu.cn-61ef6e0cd06fb9b8/jpeg-decoder-0.3.1/src/decoder.rs:294:9
8: jpeg_decoder_poc::main
at ./src/main.rs:5:11
9: core::ops::function::FnOnce::call_once
at /rustc/07688726805d5db0a4bca445a6651d09708041ea/library/core/src/ops/function.rs:250:5

Expected behavior

Not panic. It could be an error reported to the users.

Test environment:

Version: jpeg-decoder = "0.3.1"
OS: Ubuntu 20.04, 64 bit
Target triple: x86_64-unknown-linux-gnu
Rustc version: rustc 1.73.0-nightly (076887268 2023-08-17)

To Reproduce

The PoC to reproduce the bug:

fn main() {
	let p = "jpeg-decode-crash.xx";
	if let Ok(data) = std::fs::read(p) {
		let mut decoder = jpeg_decoder::Decoder::new(data.as_slice());
		let _ = decoder.decode();
	}
}

PoC input is attached:
jpeg-decode-crash.xx.zip

@quilan1
Copy link
Contributor

quilan1 commented Feb 15, 2024

From the spec:

H.1.2.1 Prediction
At the beginning of the first line and at the beginning of each restart interval the prediction value of 2^(P – 1) is used, where P is the input precision.
If the point transformation parameter (see A.4) is non-zero, the prediction value at the beginning of the first lines and the beginning of each restart interval is 2^(P – Pt – 1), where Pt is the value of the point transformation parameter.
Each prediction is calculated with full integer arithmetic precision, and without clamping of either underflow or overflow beyond the input precision bounds. For example, if Ra and Rb are both 16-bit integers, the sum is a 17-bit integer. After dividing the sum by 2 (predictor 7, [note: example refers to predictor Px = (Ra + Rb)/2]), the prediction is a 16-bit integer.
[...]
The difference between the prediction value and the input is calculated modulo 2^16. In the decoder the difference is decoded and added, modulo 2^16, to the prediction.

and, related:

A.4 Point transform
In the lossless mode of operation the point transform is applied to the input samples. In the difference coding of the
hierarchical mode of operation the point transform is applied to the difference between the input component samples and
the reference component samples. In both cases the point transform is an integer divide by 2^Pt, where Pt is the value of the
point transform parameter (see B.2.3). [...] The output of the decoder is rescaled by multiplying by 2^Pt.

The current code is (in lossless.rs):

let diff = differences[i][0]; /* as i32 */
let prediction = 1 << (frame.precision /* as u8 */ - scan.point_transform /* as u8 */ - 1) as i32;
let result = ((prediction + diff) & 0xFFFF) as u16; // modulo 2^16
let result = result << scan.point_transform;

In the example file, frame.precision is 8, scan.point_transform is 12. Simple underflow problem. I might suggest, given the wording of A.4: let prediction = (1 << (frame.precision - 1) as i32) >> scan.point_transform as i32;. This properly truncates to 0, instead of 1.

Edit: Removing the latter stuff, because I don't think that's at all what the spec was talking about. That said, I've still got a few questions about how the implementation deals with point_transform over the course of the function, but I'm going to look deeper.

@fintelia
Copy link
Contributor

Thanks for looking into this! If you figure out a fix, please do open a PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants