FLAC__stream_decoder_seek_absolute sometimes fails when it shouldn't

sudo-rm-rf-oops commented 6 years ago

Function FLAC__stream_decoder_seek_absolute sometimes returns false when the file (and the requested offset) are both absolutely fine. This is because of a bug in read_subframelpc (in stream_decoder.c) which fails to notify its caller that the decoder has not yet got into frame sync.

The faulty line of code is:

if(!read_residual_partitioned_rice_(decoder, order, subframe->entropy_coding_method.data.partitioned_rice.order, &decoder->private_->partitioned_rice_contents[channel], decoder->private_->residual[channel], /*is_extended=*/subframe->entropy_coding_method.type == FLAC__ENTROPY_CODING_METHOD_PARTITIONED_RICE2))
    return false;

And this should read (I reckon):

if(!read_residual_partitioned_rice_(decoder, order, subframe->entropy_coding_method.data.partitioned_rice.order, &decoder->private_->partitioned_rice_contents[channel], decoder->private_->residual[channel], /*is_extended=*/subframe->entropy_coding_method.type == FLAC__ENTROPY_CODING_METHOD_PARTITIONED_RICE2))
{
    send_error_to_client_(decoder, FLAC__STREAM_DECODER_ERROR_STATUS_UNPARSEABLE_STREAM);
    decoder->protected_->state = FLAC__STREAM_DECODER_SEARCH_FOR_FRAME_SYNC;
    return true;
}

Something like that, anyway. I didn't dig that deeply.

erikd commented 6 years ago

Have you tried applying that and running the tests?

sudo-rm-rf-oops commented 6 years ago

Hi Erik,

I've walked it through that code path in the debugger for my particular failing case and it seems to work but I plan to dig into the code a little more this morning so I might have a bit more to contribute. If so, I will post back here.

Paul Sanders (sadly, that name was taken!).

sudo-rm-rf-oops commented 6 years ago

Hello again,

OK, I did a bit more digging and my suggested fix was naive. I also took a copy of the latest code and tested with that (mine was older) and the problem disappeared but I'm not convinced it's fixed - a change elsewhere might just have masked it.

Looking inside read_residual_partitioned_rice_ in the code I have we find this:

/* sanity checks */
if(partition_order == 0) {
    if(decoder->private_->frame.header.blocksize < predictor_order) {
        send_error_to_client_(decoder, FLAC__STREAM_DECODER_ERROR_STATUS_LOST_SYNC);
        decoder->protected_->state = FLAC__STREAM_DECODER_SEARCH_FOR_FRAME_SYNC;
        /* We have received a potentially malicious bit stream. All we can do is error out to avoid a heap overflow. */
        return false;
    }
}
else {
    if(partition_samples < predictor_order) {
        send_error_to_client_(decoder, FLAC__STREAM_DECODER_ERROR_STATUS_LOST_SYNC);
        decoder->protected_->state = FLAC__STREAM_DECODER_SEARCH_FOR_FRAME_SYNC;
        /* We have received a potentially malicious bit stream. All we can do is error out to avoid a heap overflow. */
        return false;
    }
}

And, for that code, the correct thing to do would be to return true (falsebeing reserved for fatal errors, as I understand it) so that the caller knows to continue trying to sync to a frame.

However, in the latest code, we find this:

/* invalid predictor and partition orders mush be handled in the callers */
FLAC__ASSERT(partition_order > 0? partition_samples >= predictor_order : decoder->private_->frame.header.blocksize >= predictor_order);

Is that wise? Can this condition ever legitimately happen now (because it certainly did before)? If so, then the code should be something like:

/* invalid predictor and partition orders mush be handled in the callers */
if (! ((partition_order > 0) ? partition_samples >= predictor_order : decoder->private_->frame.header.blocksize >= predictor_order))
{
    send_error_to_client_(decoder, FLAC__STREAM_DECODER_ERROR_STATUS_LOST_SYNC);
    decoder->protected_->state = FLAC__STREAM_DECODER_SEARCH_FOR_FRAME_SYNC;
    return true;
}

What do you think? I think I'm going to edit that into my copy because I don't trust that assert. I will also see if I can get the new code to go through that code path by doing more seeks but it looks like you have improved the frame syncing logic in general so I might not succeed.

sudo-rm-rf-oops commented 6 years ago

PS: Some compiler warnings for you from Visual Studio 2017, in case you're interested:

metadata_iterators.c
libflac\metadata_iterators.c(579): warning C4244: 'return': conversion from 'const __int64' to 'off_t', possible loss of data
libflac\metadata_iterators.c(1117): warning C4244: '+=': conversion from 'const __int64' to 'uint32_t', possible loss of data
libflac\metadata_iterators.c(1129): warning C4244: '=': conversion from '__int64' to 'uint32_t', possible loss of data
libflac\metadata_iterators.c(1152): warning C4244: '-=': conversion from 'const __int64' to 'uint32_t', possible loss of data
libflac\metadata_iterators.c(1647): warning C4244: '=': conversion from '__int64' to 'uint32_t', possible loss of data
libflac\metadata_iterators.c(1652): warning C4244: '=': conversion from '__int64' to 'uint32_t', possible loss of data
libflac\metadata_iterators.c(1666): warning C4244: '=': conversion from 'const __int64' to 'uint32_t', possible loss of data

metadata_object.c
libflac\metadata_object.c(1127): warning C4244: '=': conversion from 'FLAC__uint64' to 'uint32_t', possible loss of data

neheb commented 5 years ago

This would fix most of those warnings:

--- a/src/libFLAC/metadata_iterators.c
+++ b/src/libFLAC/metadata_iterators.c
@@ -571,7 +571,7 @@ FLAC_API FLAC__bool FLAC__metadata_simple_iterator_is_last(const FLAC__Metadata_
 }

 /*@@@@add to tests*/
-FLAC_API off_t FLAC__metadata_simple_iterator_get_block_offset(const FLAC__Metadata_SimpleIterator *iterator)
+FLAC_API FLAC__off_t FLAC__metadata_simple_iterator_get_block_offset(const FLAC__Metadata_SimpleIterator *iterator)
 {
        FLAC__ASSERT(0 != iterator);
        FLAC__ASSERT(0 != iterator->file);
@@ -1633,7 +1633,7 @@ FLAC_API FLAC__bool FLAC__metadata_chain_check_if_tempfile_needed(FLAC__Metadata
         */
        FLAC__off_t current_length;
        LastBlockState lbs_state = LBS_NONE;
-       uint32_t lbs_size = 0;
+       FLAC__off_t lbs_size = 0;

        FLAC__ASSERT(0 != chain);