Re: [flac-dev] Possible overflow of _candidate_bits in stream_encoder.c
Op ma 6 jul. 2020 om 10:22 schreef Erik de Castro Lopo :
>
> 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 two PDFs 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
From 08f4931772ddb17ca0f27012a867767d87d44d7d Mon Sep 17 00:00:00 2001
From: Martijn van Beurden
Date: Mon, 6 Jul 2020 21:38:39 +0200
Subject: [PATCH 4/4] Add some overflow checks for residual bits calculation
---
src/libFLAC/stream_encoder.c | 23 +++
1 file changed, 15 insertions(+), 8 deletions(-)
diff --git a/src/libFLAC/stream_encoder.c b/src/libFLAC/stream_encoder.c
index 74387ec3..4c91247f 100644
--- a/src/libFLAC/stream_encoder.c
+++ b/src/libFLAC/stream_encoder.c
@@ -4110,13 +4110,14 @@ static inline uint32_t count_rice_bits_in_partition_(
const FLAC__int32 *residual
)
{
- uint32_t i, partition_bits =
+ uint32_t i;
+ uint64_t partition_bits =
FLAC__ENTROPY_CODING_METHOD_PARTITIONED_RICE_PARAMETER_LEN + /* actually could end up being FLAC__ENTROPY_CODING_METHOD_PARTITIONED_RICE2_PARAMETER_LEN but err on side of 16bps */
(1+rice_parameter) * partition_samples /* 1 for unary stop bit + rice_parameter for the binary portion */
;
for(i = 0; i < partition_samples; i++)
partition_bits += ( (FLAC__uint32)((residual[i]<<1)^(residual[i]>>31)) >> rice_parameter );
- return partition_bits;
+ return (uint32_t)(flac_min(partition_bits,(uint32_t)(-1))); // To make sure the return value doesn't overflow
}
#else
static inline uint32_t count_rice_bits_in_partition_(
@@ -4125,15 +4126,15 @@ static inline uint32_t count_rice_bits_in_partition_(
const FLAC__uint64 abs_residual_partition_sum
)
{
- return
+ return (uint32_t)(flac_min( // To make sure the return value doesn't overflow
FLAC__ENTROPY_CODING_METHOD_PARTITIONED_RICE_PARAMETER_LEN + /* actually could end up being FLAC__ENTROPY_CODING_METHOD_PARTITIONED_RICE2_PARAMETER_LEN but err on side of 16bps */
(1+rice_parameter) * partition_samples + /* 1 for unary stop bit + rice_parameter for the binary portion */
(
rice_parameter?
-(uint32_t)(abs_residual_partition_sum >> (rice_parameter-1)) /* rice_parameter-1 because the real coder sign-folds instead of using a sign bit */
-: (uint32_t)(abs_residual_partition_sum << 1) /* can't shift by negative number, so reverse */
+(abs_residual_partition_sum >> (rice_parameter-1)) /* rice_parameter-1 because the real coder sign-folds instead of using a sign bit */
+: (abs_residual_partition_sum << 1) /* can't shift by negative number, so reverse */
)
- - (partition_samples >> 1)
+ - (partition_samples >> 1),(uint32_t)(-1)));
/* -(partition_samples>>1) to subtract out extra contributions to the abs_residual_partition_sum.
* The actual number of bits used is closer to the sum(for all i in the partition) of abs(residual[i])>>(rice_parameter-1)
* By using the abs_residual_partition sum, we also add in bits in the LSBs that would normally be shifted out.
@@ -4224,7 +4225,10 @@ FLAC__bool set_partitioned_rice_(
raw_bits[0] = 0;
}
parameters[0] = best_rice_parameter;
- bits_ += best_partition_bits;
+ if(best_partition_bits < UINT_MAX - bits_) // To make sure _bits doesn't overflow
+ bits_ += best_partition_bits;
+ else
+ bits_ = UINT_MAX;
}
else {
uint32_t partition, residual_sample;
@@ -4327,7 +4331,10 @@ FLAC__bool set_partitioned_rice_(
raw_bits[partition] = 0;
}
parameters[partition] = best_rice_parameter;
- bits_ += best_partition_bits;
+ if(best_partition_bits < UINT_MAX - bits_) // To make sure _bits doesn't overflow
+bits_ += best_partition_bits;
+ else
+bits_ = UINT_MAX;
residual_sample += partition_samples;
}
}
--
2.20.1
long set of samples 2.pdf
Description: Adobe PDF document
long set of samples 1.pdf
Description: Adobe PDF document
___
flac-dev mailing list
flac-dev@xiph.org
http://lists.xiph.org/mailman/listinfo/flac-dev
Re: [flac-dev] Possible overflow of _candidate_bits in stream_encoder.c
Op do 9 jul. 2020 om 00:07 schreef Erik de Castro Lopo : > Would it be possible to submit these patches as a PR on Github? > I just did for the patches against the flac source. However, the flac-website github repository doesn't seem synced with the xiph one, so I can't patch that one. Kind regards, Martijn van Beurden ___ flac-dev mailing list flac-dev@xiph.org http://lists.xiph.org/mailman/listinfo/flac-dev
Re: [flac-dev] Possible overflow of _candidate_bits in stream_encoder.c
Martijn van Beurden wrote: > > 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. Would it be possible to submit these patches as a PR on Github? Thanks, Erik -- -- Erik de Castro Lopo http://www.mega-nerd.com/ ___ flac-dev mailing list flac-dev@xiph.org http://lists.xiph.org/mailman/listinfo/flac-dev
Re: [flac-dev] Possible overflow of _candidate_bits in stream_encoder.c
Op ma 6 jul. 2020 om 10:22 schreef Erik de Castro Lopo :
>
> 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.
From 08f4931772ddb17ca0f27012a867767d87d44d7d Mon Sep 17 00:00:00 2001
From: Martijn van Beurden
Date: Mon, 6 Jul 2020 21:38:39 +0200
Subject: [PATCH 4/4] Add some overflow checks for residual bits calculation
---
src/libFLAC/stream_encoder.c | 23 +++
1 file changed, 15 insertions(+), 8 deletions(-)
diff --git a/src/libFLAC/stream_encoder.c b/src/libFLAC/stream_encoder.c
index 74387ec3..4c91247f 100644
--- a/src/libFLAC/stream_encoder.c
+++ b/src/libFLAC/stream_encoder.c
@@ -4110,13 +4110,14 @@ static inline uint32_t count_rice_bits_in_partition_(
const FLAC__int32 *residual
)
{
- uint32_t i, partition_bits =
+ uint32_t i;
+ uint64_t partition_bits =
FLAC__ENTROPY_CODING_METHOD_PARTITIONED_RICE_PARAMETER_LEN + /* actually could end up being FLAC__ENTROPY_CODING_METHOD_PARTITIONED_RICE2_PARAMETER_LEN but err on side of 16bps */
(1+rice_parameter) * partition_samples /* 1 for unary stop bit + rice_parameter for the binary portion */
;
for(i = 0; i < partition_samples; i++)
partition_bits += ( (FLAC__uint32)((residual[i]<<1)^(residual[i]>>31)) >> rice_parameter );
- return partition_bits;
+ return (uint32_t)(flac_min(partition_bits,(uint32_t)(-1))); // To make sure the return value doesn't overflow
}
#else
static inline uint32_t count_rice_bits_in_partition_(
@@ -4125,15 +4126,15 @@ static inline uint32_t count_rice_bits_in_partition_(
const FLAC__uint64 abs_residual_partition_sum
)
{
- return
+ return (uint32_t)(flac_min( // To make sure the return value doesn't overflow
FLAC__ENTROPY_CODING_METHOD_PARTITIONED_RICE_PARAMETER_LEN + /* actually could end up being FLAC__ENTROPY_CODING_METHOD_PARTITIONED_RICE2_PARAMETER_LEN but err on side of 16bps */
(1+rice_parameter) * partition_samples + /* 1 for unary stop bit + rice_parameter for the binary portion */
(
rice_parameter?
-(uint32_t)(abs_residual_partition_sum >> (rice_parameter-1)) /* rice_parameter-1 because the real coder sign-folds instead of using a sign bit */
-: (uint32_t)(abs_residual_partition_sum << 1) /* can't shift by negative number, so reverse */
+(abs_residual_partition_sum >> (rice_parameter-1)) /* rice_parameter-1 because the real coder sign-folds instead of using a sign bit */
+: (abs_residual_partition_sum << 1) /* can't shift by negative number, so reverse */
)
- - (partition_samples >> 1)
+ - (partition_samples >> 1),(uint32_t)(-1)));
/* -(partition_samples>>1) to subtract out extra contributions to the abs_residual_partition_sum.
* The actual number of bits used is closer to the sum(for all i in the partition) of abs(residual[i])>>(rice_parameter-1)
* By using the abs_residual_partition sum, we also add in bits in the LSBs that would normally be shifted out.
@@ -4224,7 +4225,10 @@ FLAC__bool set_partitioned_rice_(
raw_bits[0] = 0;
}
parameters[0] = best_rice_parameter;
- bits_ += best_partition_bits;
+ if(best_partition_bits < UINT_MAX - bits_) // To make sure _bits doesn't overflow
+ bits_ += best_partition_bits;
+ else
+ bits_ = UINT_MAX;
}
else {
uint32_t partition, residual_sample;
@@ -4327,7 +4331,10 @@ FLAC__bool set_partitioned_rice_(
raw_bits[partition] = 0;
}
parameters[partition] = best_rice_parameter;
- bits_ += best_partition_bits;
+ if(best_partition_bits < UINT_MAX - bits_) // To make sure _bits doesn't overflow
+bits_ += best_partition_bits;
+ else
+bits_ = UINT_MAX;
residual_sample += partition_samples;
}
}
--
2.20.1
long set of samples 1.pdf
Description: Adobe PDF document
___
flac-dev mailing list
flac-dev@xiph.org
http://lists.xiph.org/mailman/listinfo/flac-dev
Re: [flac-dev] Possible overflow of _candidate_bits in stream_encoder.c
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/ ___ flac-dev mailing list flac-dev@xiph.org http://lists.xiph.org/mailman/listinfo/flac-dev
