libFLAC: Restrict number of bytes in a frame

[NeelkamalSemwal]

[ktmf01]

Could you perhaps explain what this PR should fix?

[ktmf01]

I still haven't figured out what this patch does exactly (seems viewing the bug report requires privileges) but some comments, earlier versions and reviewing can be found here, under comments: https://android-review.googlesource.com/c/platform/external/flac/+/1770797

[ktmf01]

I hadn't noticed it before, but this patch gives rise to two compiler warnings:

stream_decoder.c:3178:74: warning: comparison of integer expressions of different signedness: 'FLAC__int64' {aka 'long long int'} and 'FLAC__uint64' {aka 'long long unsigned int'} [-Wsign-compare]
 3178 |                         approx_bytes_per_frame = (approx_bytes_per_frame > (upper_bound - lower_bound)) ? (upper_bound - lower_bound) : approx_bytes_per_frame ? approx_bytes_per_frame * 2 : 16;
      |                                                                          ^
stream_decoder.c:3178:137: warning: operand of '?:' changes signedness from 'FLAC__int64' {aka 'long long int'} to 'FLAC__uint64' {aka 'long long unsigned int'} due to unsignedness of other operand [-Wsign-compare]
 3178 |                         approx_bytes_per_frame = (approx_bytes_per_frame > (upper_bound - lower_bound)) ? (upper_bound - lower_bound) : approx_bytes_per_frame ? approx_bytes_per_frame * 2 : 16;
      |                                                                                                                                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[NeelkamalSemwal]

@ktmf01 According to the original bug report, when trying to seek to an absolute sample, approx_bytes_per_frame is updated without any check i.e. approx_bytes_per_frame = approx_bytes_per_frame ? approx_bytes_per_frame * 2 : 16 This causes an integer overflow in approx_bytes_per_frame .. The PR fixes it by defining approx_bytes_per_frame as a 64-bit number (since FLAC frame sizes can be huge) and limiting the search to (upper_bound - lower_bound).

[ktmf01]

I went through this patch again (and the comments on earlier versions in https://android-review.googlesource.com/c/platform/external/flac/+/1770797) and I still don't understand where this comes from.

In theory the maximum size of a FLAC frame is unbounded, but when regarding sane frames (frames that aren't several times larger than the raw data they code for), the maximum size is max blocksize * max bits per sample * max channels which evaluated to 65535 * 32 * 8 which is approximately 2^24. So, if approx_bytes_per_frame overflows, something is definitely wrong. I therefore think that your earlier solution, which is throwing a seek error on approx_bytes_per_frame > (UINT32_MAX >> 1)) is actually a better one. You could even argue that approx_bytes_per_frame > (1 << 25) is already cause for alarm.

What I also do not understand is why this patch undoes 159cd6c. Is that by mistake?

[ktmf01]

@NeelkamalSemwal There have been quite a few changes to the seeking code recently, could you try whether the problem is already solved without this PR? If not, could you consider the changes as proposed in my previous post?

[NeelkamalSemwal]

I went through this patch again (and the comments on earlier versions in https://android-review.googlesource.com/c/platform/external/flac/+/1770797) and I still don't understand where this comes from.

In theory the maximum size of a FLAC frame is unbounded, but when regarding sane frames (frames that aren't several times larger than the raw data they code for), the maximum size is max blocksize * max bits per sample * max channels which evaluated to 65535 * 32 * 8 which is approximately 2^24. So, if approx_bytes_per_frame overflows, something is definitely wrong. I therefore think that your earlier solution, which is throwing a seek error on approx_bytes_per_frame > (UINT32_MAX >> 1)) is actually a better one. You could even argue that approx_bytes_per_frame > (1 << 25) is already cause for alarm.

What I also do not understand is why this patch undoes 159cd6c. Is that by mistake?

Yes, this patch removed your changes by mistake as this CL is ported from Android where your CL's changes are not present. https://cs.android.com/android/platform/superproject/+/master:external/flac/src/libFLAC/stream_decoder.c;drc=027e439d519a341d233a337fde415245ea4538b0;l=3034

I will modify my PR, including your changes.

[ktmf01]

In theory the maximum size of a FLAC frame is unbounded, but when regarding sane frames (frames that aren't several times larger than the raw data they code for), the maximum size is max blocksize * max bits per sample * max channels which evaluated to 65535 * 32 * 8 which is approximately 2^24. So, if approx_bytes_per_frame overflows, something is definitely wrong. I therefore think that your earlier solution, which is throwing a seek error on approx_bytes_per_frame > (UINT32_MAX >> 1)) is actually a better one. You could even argue that approx_bytes_per_frame > (1 << 25) is already cause for alarm.

I just noticed bits and bytes are mixed up here. Max framesize in bits is 65535 * 32 * 8 which is approximately 2^24, but max framesize in bytes is 65535 * 4 * 8 which is approximately 2^21. This is the maximum 'sane' framesize.

Furthermore, the field that codes for the maximum framesize in the streaminfo metadat block is 24 bits. I would argue that throwing a seek error on approx_bytes_per_frame > (1 << 24) is therefore perfectly reasonable.

[ktmf01]

@NeelkamalSemwal Do you still intend to update this PR or can I close it?

I think the best fix is by comparing approx_bytes_per_frame with

(1 << FLAC__STREAM_METADATA_STREAMINFO_MAX_FRAME_SIZE_LEN)