Re: [FFmpeg-devel] [libav-devel] [PATCH] alsdec: validate time diff index
On Wed, Apr 22, 2015 at 10:59:29AM +0200, Thilo Borgmann wrote: Am 21.04.15 um 19:35 schrieb Andreas Cadhalpun: On 21.04.2015 08:14, Thilo Borgmann wrote: Am 20.04.15 um 23:20 schrieb Andreas Cadhalpun: On 19.04.2015 22:20, Luca Barbato wrote: I'd check that `master` is always between `raw_buffer` and the end of it. You mean something like the attached patch? (I'm not sure if `div_blocks` is validated before, same for `offset`) That should catch problems in those as well. Have you tested with fate after applying this patch locally? Yes, but only on amd64, where fate passes. Attached is a new patch, which should work everywhere. LGTM applied thanks [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Frequently ignored answer#1 FFmpeg bugs should be sent to our bugtracker. User questions about the command line tools should be sent to the ffmpeg-user ML. And questions about how to use libav* should be sent to the libav-user ML. signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [libav-devel] [PATCH] alsdec: validate time diff index
Am 21.04.15 um 19:35 schrieb Andreas Cadhalpun: On 21.04.2015 08:14, Thilo Borgmann wrote: Am 20.04.15 um 23:20 schrieb Andreas Cadhalpun: On 19.04.2015 22:20, Luca Barbato wrote: I'd check that `master` is always between `raw_buffer` and the end of it. You mean something like the attached patch? (I'm not sure if `div_blocks` is validated before, same for `offset`) That should catch problems in those as well. Have you tested with fate after applying this patch locally? Yes, but only on amd64, where fate passes. Attached is a new patch, which should work everywhere. LGTM -Thilo ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [libav-devel] [PATCH] alsdec: validate time diff index
Am 20.04.15 um 23:20 schrieb Andreas Cadhalpun: On 19.04.2015 22:20, Luca Barbato wrote: On 18/04/15 18:58, Andreas Cadhalpun wrote: If begin is smaller than t, the subtraction 'begin -= t' wraps around, because begin is unsigned. The same applies for end t. This causes segmentation faults. Actually, the access to raw_buffer seems a bit optimistic all over this code. I'd check that `master` is always between `raw_buffer` and the end of it. You mean something like the attached patch? (I'm not sure if `div_blocks` is validated before, same for `offset`) That should catch problems in those as well. Have you tested with fate after applying this patch locally? -Thilo ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [libav-devel] [PATCH] alsdec: validate time diff index
On 21.04.2015 08:14, Thilo Borgmann wrote:
Am 20.04.15 um 23:20 schrieb Andreas Cadhalpun:
On 19.04.2015 22:20, Luca Barbato wrote:
I'd check that `master` is always between `raw_buffer` and the end of it.
You mean something like the attached patch?
(I'm not sure if `div_blocks` is validated before, same for `offset`)
That should catch problems in those as well.
Have you tested with fate after applying this patch locally?
Yes, but only on amd64, where fate passes.
Attached is a new patch, which should work everywhere.
Best regards,
Andreas
From 9063f33f9787940175428a76a79995bbfee34dd7 Mon Sep 17 00:00:00 2001
From: Andreas Cadhalpun andreas.cadhal...@googlemail.com
Date: Tue, 21 Apr 2015 19:25:50 +0200
Subject: [PATCH] alsdec: check sample pointer range in
revert_channel_correlation
Also change the type of begin, end and smp to ptrdiff_t to make the
comparison well-defined.
Signed-off-by: Andreas Cadhalpun andreas.cadhal...@googlemail.com
---
libavcodec/alsdec.c | 34 +++---
1 file changed, 27 insertions(+), 7 deletions(-)
diff --git a/libavcodec/alsdec.c b/libavcodec/alsdec.c
index c81db18..e34cf6e 100644
--- a/libavcodec/alsdec.c
+++ b/libavcodec/alsdec.c
@@ -1246,6 +1246,7 @@ static int revert_channel_correlation(ALSDecContext *ctx, ALSBlockData *bd,
ALSChannelData *ch = cd[c];
unsigned int dep = 0;
unsigned int channels = ctx-avctx-channels;
+unsigned int channel_size = ctx-sconf.frame_length + ctx-sconf.max_order;
if (reverted[c])
return 0;
@@ -1276,9 +1277,9 @@ static int revert_channel_correlation(ALSDecContext *ctx, ALSBlockData *bd,
bd-raw_samples = ctx-raw_samples[c] + offset;
for (dep = 0; !ch[dep].stop_flag; dep++) {
-unsigned int smp;
-unsigned int begin = 1;
-unsigned int end = bd-block_length - 1;
+ptrdiff_t smp;
+ptrdiff_t begin = 1;
+ptrdiff_t end = bd-block_length - 1;
int64_t y;
int32_t *master = ctx-raw_samples[ch[dep].master_channel] + offset;
@@ -1290,19 +1291,28 @@ static int revert_channel_correlation(ALSDecContext *ctx, ALSBlockData *bd,
if (ch[dep].time_diff_sign) {
t = -t;
-if (t 0 begin t) {
-av_log(ctx-avctx, AV_LOG_ERROR, begin %u smaller than time diff index %d.\n, begin, t);
+if (begin t) {
+av_log(ctx-avctx, AV_LOG_ERROR, begin %td smaller than time diff index %d.\n, begin, t);
return AVERROR_INVALIDDATA;
}
begin -= t;
} else {
-if (t 0 end t) {
-av_log(ctx-avctx, AV_LOG_ERROR, end %u smaller than time diff index %d.\n, end, t);
+if (end t) {
+av_log(ctx-avctx, AV_LOG_ERROR, end %td smaller than time diff index %d.\n, end, t);
return AVERROR_INVALIDDATA;
}
end -= t;
}
+if (FFMIN(begin - 1, begin - 1 + t) ctx-raw_buffer - master ||
+FFMAX(end + 1, end + 1 + t) ctx-raw_buffer + channels * channel_size - master) {
+av_log(ctx-avctx, AV_LOG_ERROR,
+ sample pointer range [%p, %p] not contained in raw_buffer [%p, %p].\n,
+ master + FFMIN(begin - 1, begin - 1 + t), master + FFMAX(end + 1, end + 1 + t),
+ ctx-raw_buffer, ctx-raw_buffer + channels * channel_size);
+return AVERROR_INVALIDDATA;
+}
+
for (smp = begin; smp end; smp++) {
y = (1 6) +
MUL64(ch[dep].weighting[0], master[smp - 1]) +
@@ -1315,6 +1325,16 @@ static int revert_channel_correlation(ALSDecContext *ctx, ALSBlockData *bd,
bd-raw_samples[smp] += y 7;
}
} else {
+
+if (begin - 1 ctx-raw_buffer - master ||
+end + 1 ctx-raw_buffer + channels * channel_size - master) {
+av_log(ctx-avctx, AV_LOG_ERROR,
+ sample pointer range [%p, %p] not contained in raw_buffer [%p, %p].\n,
+ master + begin - 1, master + end + 1,
+ ctx-raw_buffer, ctx-raw_buffer + channels * channel_size);
+return AVERROR_INVALIDDATA;
+}
+
for (smp = begin; smp end; smp++) {
y = (1 6) +
MUL64(ch[dep].weighting[0], master[smp - 1]) +
--
2.1.4
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [libav-devel] [PATCH] alsdec: validate time diff index
On 19.04.2015 22:20, Luca Barbato wrote:
On 18/04/15 18:58, Andreas Cadhalpun wrote:
If begin is smaller than t, the subtraction 'begin -= t' wraps around,
because begin is unsigned. The same applies for end t.
This causes segmentation faults.
Actually, the access to raw_buffer seems a bit optimistic all over this
code.
I'd check that `master` is always between `raw_buffer` and the end of it.
You mean something like the attached patch?
(I'm not sure if `div_blocks` is validated before, same for `offset`)
That should catch problems in those as well.
Best regards,
Andreas
From 5b0a985130f94c887c40028f5549a29576a26991 Mon Sep 17 00:00:00 2001
From: Andreas Cadhalpun andreas.cadhal...@googlemail.com
Date: Mon, 20 Apr 2015 23:14:28 +0200
Subject: [PATCH] alsdec: check sample pointer range in
revert_channel_correlation
Signed-off-by: Andreas Cadhalpun andreas.cadhal...@googlemail.com
---
libavcodec/alsdec.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/libavcodec/alsdec.c b/libavcodec/alsdec.c
index c81db18..a14761c 100644
--- a/libavcodec/alsdec.c
+++ b/libavcodec/alsdec.c
@@ -1246,6 +1246,7 @@ static int revert_channel_correlation(ALSDecContext *ctx, ALSBlockData *bd,
ALSChannelData *ch = cd[c];
unsigned int dep = 0;
unsigned int channels = ctx-avctx-channels;
+unsigned int channel_size = ctx-sconf.frame_length + ctx-sconf.max_order;
if (reverted[c])
return 0;
@@ -1303,6 +1304,15 @@ static int revert_channel_correlation(ALSDecContext *ctx, ALSBlockData *bd,
end -= t;
}
+if (master + FFMIN(begin - 1, begin - 1 + t) ctx-raw_buffer ||
+master + FFMAX(end + 1, end + 1 + t)ctx-raw_buffer + channels * channel_size) {
+av_log(ctx-avctx, AV_LOG_ERROR,
+ sample pointer range [%p, %p] not contained in raw_buffer [%p, %p].\n,
+ master + FFMIN(begin - 1, begin - 1 + t), master + FFMAX(end + 1, end + 1 + t),
+ ctx-raw_buffer, ctx-raw_buffer + channels * channel_size);
+return AVERROR_INVALIDDATA;
+}
+
for (smp = begin; smp end; smp++) {
y = (1 6) +
MUL64(ch[dep].weighting[0], master[smp - 1]) +
--
2.1.4
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [libav-devel] [PATCH] alsdec: validate time diff index
On 18.04.2015 21:55, Luca Barbato wrote: On 18/04/15 18:58, Andreas Cadhalpun wrote: If begin is smaller than t, the subtraction 'begin -= t' wraps around, because begin is unsigned. The same applies for end t. Why that variable is unsigned? Probably because it should never be negative. This causes segmentation faults. Do you have a sample to trigger it? Yes, several. Best regards, Andreas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
