Re: [FFmpeg-devel] [PATCH 1/2] avformat/icodec: ico probe with unknown data
On Tue, Feb 16, 2016 at 01:42:50AM -0800, Mark Harris wrote: > >> Header: OK, 2 frames > >> Frame 0: Unknown (offset points beyond end of probe buffer) > >> Frame 1: Invalid > >> Previously this example had a score of 25, even though the score would > >> be 1 if the unknown frame was known to be valid or 0 if it was known > >> to be invalid. For this example the score is now 1. > > > > If the header is ok and the offset points beyond the end > > of the probe buffer, the score was always 25 (before I > > extended the check). Why should this be changed? > > 25 is not very high after checking ~40 bit. > > If you think that 25 is better when there is 1 unknown frame and 1 > invalid frame, I don't see an issue with that. However in that case, > when the unknown frame is known to be valid it should have a score > that is at least the same value. It doesn't make sense for the score > with an unknown value to be higher than the highest score that could > possibly be produced if the unknown value was known. > > For this patch I just wanted to keep the behavior exactly the same in > the common case where all of the data is in the buffer. > > For the case of unknown data, the existing code can do strange things. > For example if the probe buffer ends in the middle of a size or offset > field then it can end up reading a partial value, which can cause it > to check bytes at the wrong offset or produce false positives or false > negatives. There were also inconsistencies if there was a mixture of > unknown and invalid data. It seemed only logical that with unknown > data the score should never be higher than what it would be if the > data was known to be valid, and never be lower than what it would be > if the data was known to be invalid. So I preserved the existing > behavior for the cases of known data, and adjusted the behavior for > unknown data so that the scores would be within these constraints and > so that there was no reliance on the zero padding. patch applied the hardcoded litteral offsets could be replaced by named constants or comments be added also the score should possibly be higher for truncated files, it would be better if a decissive recognization could be done before the whole file needs to be read thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The real ebay dictionary, page 1 "Used only once"- "Some unspecified defect prevented a second use" "In good condition" - "Can be repaird by experienced expert" "As is" - "You wouldnt want it even if you were payed for it, if you knew ..." signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/2] avformat/icodec: ico probe with unknown data
>> Header: OK, 2 frames >> Frame 0: Unknown (offset points beyond end of probe buffer) >> Frame 1: Invalid >> Previously this example had a score of 25, even though the score would >> be 1 if the unknown frame was known to be valid or 0 if it was known >> to be invalid. For this example the score is now 1. > > If the header is ok and the offset points beyond the end > of the probe buffer, the score was always 25 (before I > extended the check). Why should this be changed? > 25 is not very high after checking ~40 bit. If you think that 25 is better when there is 1 unknown frame and 1 invalid frame, I don't see an issue with that. However in that case, when the unknown frame is known to be valid it should have a score that is at least the same value. It doesn't make sense for the score with an unknown value to be higher than the highest score that could possibly be produced if the unknown value was known. For this patch I just wanted to keep the behavior exactly the same in the common case where all of the data is in the buffer. For the case of unknown data, the existing code can do strange things. For example if the probe buffer ends in the middle of a size or offset field then it can end up reading a partial value, which can cause it to check bytes at the wrong offset or produce false positives or false negatives. There were also inconsistencies if there was a mixture of unknown and invalid data. It seemed only logical that with unknown data the score should never be higher than what it would be if the data was known to be valid, and never be lower than what it would be if the data was known to be invalid. So I preserved the existing behavior for the cases of known data, and adjusted the behavior for unknown data so that the scores would be within these constraints and so that there was no reliance on the zero padding. - Mark ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/2] avformat/icodec: ico probe with unknown data
Mark Harris gmail.com> writes: > Header: OK, 2 frames > Frame 0: Unknown (offset points beyond end of probe buffer) > Frame 1: Invalid > Previously this example had a score of 25, even though the score would > be 1 if the unknown frame was known to be valid or 0 if it was known > to be invalid. For this example the score is now 1. If the header is ok and the offset points beyond the end of the probe buffer, the score was always 25 (before I extended the check). Why should this be changed? 25 is not very high after checking ~40 bit. Carl Eugen ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 1/2] avformat/icodec: ico probe with unknown data
Fix cases where unknown data (data beyond p->buf_size) could produce a
higher ico probe score than if the unknown data was known and valid.
For example:
Header: OK, 2 frames
Frame 0: Unknown (offset points beyond end of probe buffer)
Frame 1: Invalid
Previously this example had a score of 25, even though the score would
be 1 if the unknown frame was known to be valid or 0 if it was known
to be invalid. For this example the score is now 1.
---
libavformat/icodec.c | 16 ++--
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/libavformat/icodec.c b/libavformat/icodec.c
index 6ddb901..b247cb2 100644
--- a/libavformat/icodec.c
+++ b/libavformat/icodec.c
@@ -45,11 +45,14 @@ typedef struct {
static int probe(AVProbeData *p)
{
-unsigned i, frames = AV_RL16(p->buf + 4);
+unsigned i, frames, checked = 0;
-if (AV_RL16(p->buf) || AV_RL16(p->buf + 2) != 1 || !frames)
+if (p->buf_size < 22 || AV_RL16(p->buf) || AV_RL16(p->buf + 2) != 1)
return 0;
-for (i = 0; i < frames; i++) {
+frames = AV_RL16(p->buf + 4);
+if (!frames)
+return 0;
+for (i = 0; i < frames && i * 16 + 22 <= p->buf_size; i++) {
unsigned offset;
if (AV_RL16(p->buf + 10 + i * 16) & ~1)
return FFMIN(i, AVPROBE_SCORE_MAX / 4);
@@ -61,13 +64,14 @@ static int probe(AVProbeData *p)
if (offset < 22)
return FFMIN(i, AVPROBE_SCORE_MAX / 4);
if (offset + 8 > p->buf_size)
-return AVPROBE_SCORE_MAX / 4 + FFMIN(i, 1);
+continue;
if (p->buf[offset] != 40 && AV_RB64(p->buf + offset) != PNGSIG)
return FFMIN(i, AVPROBE_SCORE_MAX / 4);
-if (i * 16 + 6 > p->buf_size)
-return AVPROBE_SCORE_MAX / 4 + FFMIN(i, 1);
+checked++;
}
+if (checked < frames)
+return AVPROBE_SCORE_MAX / 4 + FFMIN(checked, 1);
return AVPROBE_SCORE_MAX / 2 + 1;
}
--
2.7.1
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
