Shnatsel
commented
4 years ago FYI cargo afl build actually compiles with optimizations but with debug assertions and overflow checks enabled, so it's much faster than true debug mode.
Open dbrgn opened 4 years ago
Shnatsel
commented
4 years ago FYI cargo afl build actually compiles with optimizations but with debug assertions and overflow checks enabled, so it's much faster than true debug mode.
dbrgn
commented
4 years ago Ah, interesting. What's the difference between cargo afl build and cargo afl build --release then? Will the latter exclude debug assertions / overflow checks?
Shnatsel
commented
4 years ago I believe the latter will, yes. I'm not 100% sure, test it if you need to rely on that behavior.
fintelia
commented
4 years ago The panic in happening on this line. I don't know the code well enough to say what is going on there or what the right way to handle underflow would be, but it kind of looks like it is trying to compute coefficients[index] = coefficients[index] | bit?
quilan1
commented
3 years ago The panic in happening on this line. I don't know the code well enough to say what is going on there or what the right way to handle underflow would be, but it kind of looks like it is trying to compute
coefficients[index] = coefficients[index] | bit?
This is an incorrect reading, although it would make things a lot easier if it were the case. [edit: stuff removed, see next post for a more correct analysis]
To look at the spec:
G.1.2.3 Coding model for subsequent scans of successive approximation [...] The run length-magnitude composite value is Huffman coded and each Huffman code is followed by additional bits: a) One bit codes the sign of the newly non-zero coefficient. A 0-bit codes a negative sign; a 1-bit codes a positive sign. b) For each coefficient with a non-zero history, one bit is used to code the correction. A 0-bit means no correction and a 1-bit means that one shall be added to the (scaled) decoded magnitude of the coefficient.
All that said, I think the current code may be incorrect:
The current code is as follows:
// On coefficients with a non-zero history
// Note: `bit` is (1<<successive_approximation_low)
else if huffman.get_bits(reader, 1)? == 1 && coefficients[index] & bit == 0 {
if coefficients[index] > 0 {
coefficients[index] += bit;
}
else {
coefficients[index] -= bit;
}
}I believe the check on coefficients[index] & bit == 0 is potentially erroneous, as e.g. if the coefficient is +1 and the adjustment is +1, it should (as I read it) result in +2, instead of remaining at +1. [edit: stuff removed, see next post]
This is a RAW (read-as-written) rather than likely RAI (read-as-intended) interpretation though. I think the RAI is that subsequent approximation passes add on individual bits (the current code); this appears to be the guidance in the spec for encoders to follow. A RAW interpretation is that a poor encoder could encode multiple passes on the same bits, to incrementally increase them or decrease them. I guess this really depends upon how we'd like to handle weird cases like this. Simple encoders would likely be fine with the existing code, but unless I'm reading the spec wrong, I can see cases where this wouldn't be applicable.
As for OP's issue, even though it's clearly a pathological case, I think the expected operation here would be a saturating_add / saturating_sub, no?
quilan1
commented
3 years ago Actually, further reading clarified things a bit, apologies. It looks like there are no cases where positive coefficients would be given a negative correction value, or vice-versa.
The first pass (zero-history) should always look like:
Negative value: Huff[XXXX0001] + 0 + 1 => a value of -(1<<successive_approximation_low)
Positive value: Huff[XXXX0001] + 1 + 1 => a value of (1 << successive_approximation_low)
Non-zero history should always look like:
Negative history, increment: 1 => a value of prev_value - (1 << successive_approximation_low)
Positive history, increment: 1 => a value of prev_value + (1 << successive_approximation_low)
I guess we'll consider each case then for the non-zero history:
Negative previous value: 0xFFFE (-2), magnitude increment of 1: result should be 0xFFFD (-3).
Positive previous value: 0x0002 (+2), magnitude increment of 1: result should be 0x0003 (+3).
In the negative coefficient case, a simple | would not yield the correct value.
Decoding the following input file crashes in debug mode (but not in release mode, because overflows silently wrap in release mode).
id:000000,sig:06,src:000000,op:flip2,pos:1027.tar.gz
Found using afl (see #131).
Potentially related to #63.