flac dev - Jul 2020 - Possible overflow of _candidate_bits in stream_encoder.c

Recently I was trying some new approaches to improve FLAC compression, when
I stumbled on a possible overflow. The reason this has not come up earlier
is because the encoder only hits this point when the estimate of the
rice_parameter is very much off.

To trigger this overflow, one has to force rice_parameter to 0 in for
example the function evaluate_lpc_subframe in libFLAC/stream_encoder.c.
When encoding noisy material, which needs a high rice parameter, it can
happen that the return value of that function overflows. When this happens,
the wrong order might be picked, and the file blows up to enormous
proportions. In my case, about 30 times the size of the original WAV file.

When analysing this problem, I found more or less the same overflow in
flac/analyse.c, where in flac__analyse_frame, frame_bytes is multiplied by
8.

Should I send a patch to change all affected uint32_t to uint64_t? Or is
this benign enough not to matter? As far as I can tell, such a patch should
only touch private functions, no public ones.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.xiph.org/pipermail/flac-dev/attachments/20200702/5b21514f/attachment.html>

Martijn van Beurden wrote:
> To trigger this overflow, one has to force rice_parameter to 0
Ok, that sounds dodgy.
> in for
> example the function evaluate_lpc_subframe in libFLAC/stream_encoder.c.
> When encoding noisy material, which needs a high rice parameter, it can
> happen that the return value of that function overflows.
Te value is currently uint32_t ?
> Should I send a patch to change all affected uint32_t to uint64_t? Or is
> this benign enough not to matter? As far as I can tell, such a patch should
> only touch private functions, no public ones.
Would be interested if there is any measureable difference in performance
from this change.

Erik
-- 
----------------------------------------------------------------------
Erik de Castro Lopo
http://www.mega-nerd.com/

Op ma 6 jul. 2020 om 10:22 schreef Erik de Castro Lopo <mle+la at
mega-nerd.com>:
>
> Martijn van Beurden wrote:
>
> > To trigger this overflow, one has to force rice_parameter to 0
>
> Ok, that sounds dodgy.
Yes, well, it is. It could very well be that without patching, nobody
ever has a problem with this, but as the rice code is based on an
estimate, it might, perhaps. Especially if someone reenables
rice_parameter_search, which is currently marked as deprecated.
Patching it doesn't seem to affect speed in a measurable way. But I
would very well understand if these two patches are not accepted.

Attached is a patch and a PDF with a comparison of the current git
versus application of the 4 patches I sent today and yesterday. These
tests have been run on a Raspberry Pi B 3+, and as can be seen from
comparing the two files, there is quite a bit of measurement
uncertainty in decoding. It seems to me this patch doesn't change the
encoding speed, and the analyse.c patch doesn't change the decoding
speed.

Kind regards, Martijn van Beurden

P.S.: I'm sending this for the second time, sorry if it arrives twice.
It seems to me e-mails over 50kb don't get through.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-Add-some-overflow-checks-for-residual-bits-calculati.patch
Type: text/x-patch
Size: 3540 bytes
Desc: not available
URL:
<http://lists.xiph.org/pipermail/flac-dev/attachments/20200706/840cef4a/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: long set of samples 1.pdf
Type: application/pdf
Size: 19595 bytes
Desc: not available
URL:
<http://lists.xiph.org/pipermail/flac-dev/attachments/20200706/840cef4a/attachment-0001.pdf>