flac dev - Jul 2013 - exhaustive-model-search issue results in multi-gigabyte FLAC file

Martijn van Beurden wrote:
> You've exposed at least two very serious FLAC bugs, 
> namely a malfunctioning RICE2-partition encoder and a bug concerning 
> choosing verbatim frames over fixed/lpc frames.
The second, not choosing verbatim frames over fixed/lpc frames is almost
certainly a direct result of the first problem.

The fix was changing one local variable from FLAC_uint32 to FLAC_uint64
in function precompute_partition_info_sums_().

   
https://git.xiph.org/?p=flac.git;a=commit;h=6f7ec60c7e7f05f5ab0b1cf6b7b0945e44afcd4b

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

On Wed, Jul 17, 2013 at 07:45:53PM +1000, Erik de Castro Lopo
wrote:
> The fix was changing one local variable from FLAC_uint32 to FLAC_uint64
> in function precompute_partition_info_sums_().
> 
>    
https://git.xiph.org/?p=flac.git;a=commit;h=6f7ec60c7e7f05f5ab0b1cf6b7b0945e44afcd4b
I don't like this fix. It will probably hurt performance with 16-bit
data and it won't fix the problem in the assembly code.

I think the check if 32-bit accumulator is enough should be improved
instead if possible.

-- 
Miroslav Lichvar

Miroslav Lichvar wrote:
> On Wed, Jul 17, 2013 at 07:45:53PM +1000, Erik de Castro Lopo wrote:
> > The fix was changing one local variable from FLAC_uint32 to
FLAC_uint64
> > in function precompute_partition_info_sums_().
> > 
> >    
https://git.xiph.org/?p=flac.git;a=commit;h=6f7ec60c7e7f05f5ab0b1cf6b7b0945e44afcd4b
> 
> I don't like this fix. It will probably hurt performance with 16-bit
> data and it won't fix the problem in the assembly code.
> 
> I think the check if 32-bit accumulator is enough should be improved
> instead if possible.
I'm currently working on a test for this issue. Once we have a test
in the test suite I would be happy to accept patches that improve
performance.

I like to make it correct before I make it fast.

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

On 17/07/13 19:45, Erik de Castro Lopo wrote:
> Martijn van Beurden wrote:
>
>> You've exposed at least two very serious FLAC bugs,
>> namely a malfunctioning RICE2-partition encoder and a bug concerning
>> choosing verbatim frames over fixed/lpc frames.
>
> The second, not choosing verbatim frames over fixed/lpc frames is almost
> certainly a direct result of the first problem.
>
> The fix was changing one local variable from FLAC_uint32 to FLAC_uint64
> in function precompute_partition_info_sums_().
>
>     
https://git.xiph.org/?p=flac.git;a=commit;h=6f7ec60c7e7f05f5ab0b1cf6b7b0945e44afcd4b
Great! After updating from git, I can confirm that both the snippet that 
I posted here, and the full original track, both encode as expected 
using -7.

Thanks
Leigh

Miroslav Lichvar wrote:
> On Wed, Jul 17, 2013 at 07:45:53PM +1000, Erik de Castro Lopo wrote:
> > The fix was changing one local variable from FLAC_uint32 to
FLAC_uint64
> > in function precompute_partition_info_sums_().
> > 
> >    
https://git.xiph.org/?p=flac.git;a=commit;h=6f7ec60c7e7f05f5ab0b1cf6b7b0945e44afcd4b
> 
> I don't like this fix. It will probably hurt performance with 16-bit
> data and it won't fix the problem in the assembly code.
> 
> I think the check if 32-bit accumulator is enough should be improved
> instead if possible.
Miroslav,

I have committed an improvement on the above fix.

   
https://git.xiph.org/?p=flac.git;a=commit;h=f34f31dac0032887887b5bbcb0944de055b757d0

that reverts to the use of a FLAC_uint32 accumulator for files of less
than 24 bits per sample.

I still have no proof that this overflow cannot occur for 16 bit files.

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