[PATCH]lavc/pnmdec: Do not fail by default for truncated pbm files
Hi! Some scanner - scannersoftware combination (HP_Officejet_Pro_X476dn_MFP - sane) truncates trailing zeros in pbm files and FreeImage (a library used in free and proprietary image software) reads such files. Attached patch copies this behaviour and fixes ticket #5795. Please comment, Carl Eugen
On Sun, Sep 04, 2016 at 08:58:44PM +0200, Carl Eugen Hoyos wrote:
Hi!
Some scanner - scannersoftware combination (HP_Officejet_Pro_X476dn_MFP - sane) truncates trailing zeros in pbm files and FreeImage (a library used in free and proprietary image software) reads such files. Attached patch copies this behaviour and fixes ticket #5795.
Please comment, Carl Eugen
pnmdec.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) fd31d6d029cade2e5eb2f2a705067cdaa2ab02d5 0001-lavc-pnmdec-Do-not-abort-by-default-for-truncated-pb.patch From af00c56b38b28e07bbba46031472da41300a8cf1 Mon Sep 17 00:00:00 2001 From: Carl Eugen Hoyos <cehoyos@ag.or.at> Date: Sun, 4 Sep 2016 20:52:28 +0200 Subject: [PATCH] lavc/pnmdec: Do not abort by default for truncated pbm files.
Fixes ticket #5795. --- libavcodec/pnmdec.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/libavcodec/pnmdec.c b/libavcodec/pnmdec.c index d4261a4..0b7a0f6 100644 --- a/libavcodec/pnmdec.c +++ b/libavcodec/pnmdec.c @@ -124,8 +124,12 @@ static int pnm_decode_frame(AVCodecContext *avctx, void *data, do_read: ptr = p->data[0]; linesize = p->linesize[0]; - if (s->bytestream + n * avctx->height > s->bytestream_end) - return AVERROR_INVALIDDATA; + if (s->bytestream + n * avctx->height > s->bytestream_end) { + av_log(avctx, AV_LOG_ERROR, + "Invalid truncated file\n"); + if (avctx->strict_std_compliance >= FF_COMPLIANCE_STRICT) + return AVERROR_INVALIDDATA; + } if(s->type < 4 || (is_mono && s->type==7)){ for (i=0; i<avctx->height; i++) { PutBitContext pb;
@@ -159,6 +163,8 @@ static int pnm_decode_frame(AVCodecContext *avctx, void *data, } }else{ for (i = 0; i < avctx->height; i++) { + if (s->bytestream + n > s->bytestream_end) + continue;
having a pointer point outside of 0..array length is undefined behaviour (and can overflow in principle) [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Those who would give up essential Liberty, to purchase a little temporary Safety, deserve neither Liberty nor Safety -- Benjamin Franklin
2016-09-05 9:21 GMT+02:00 Michael Niedermayer <michael@niedermayer.cc>:
On Sun, Sep 04, 2016 at 08:58:44PM +0200, Carl Eugen Hoyos wrote:
@@ -159,6 +163,8 @@ static int pnm_decode_frame(AVCodecContext *avctx, void *data, } }else{ for (i = 0; i < avctx->height; i++) { + if (s->bytestream + n > s->bytestream_end) + continue;
having a pointer point outside of 0..array length is undefined behaviour (and can overflow in principle)
New patch attached. Thank you, Carl Eugen
On 9/5/16, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
2016-09-05 9:21 GMT+02:00 Michael Niedermayer <michael@niedermayer.cc>:
On Sun, Sep 04, 2016 at 08:58:44PM +0200, Carl Eugen Hoyos wrote:
@@ -159,6 +163,8 @@ static int pnm_decode_frame(AVCodecContext *avctx, void *data, } }else{ for (i = 0; i < avctx->height; i++) { + if (s->bytestream + n > s->bytestream_end) + continue;
having a pointer point outside of 0..array length is undefined behaviour (and can overflow in principle)
New patch attached.
It seems this patch disables check for all cases when experimental is enabled, but check for overflow in only one case.
2016-09-05 10:26 GMT+02:00 Paul B Mahol <onemda@gmail.com>:
On 9/5/16, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
New patch attached.
It seems this patch disables check for all cases when experimental is enabled, but check for overflow in only one case.
I am not sure I understand: Do you mean I missed a case where an overflow is now (after the patch) possible (but wasn't before) or do you mean there are formats after the patch that allow truncation and formats that do not allow it? Carl Eugen
2016-09-05 11:12 GMT+02:00 Carl Eugen Hoyos <ceffmpeg@gmail.com>:
2016-09-05 10:26 GMT+02:00 Paul B Mahol <onemda@gmail.com>:
On 9/5/16, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
New patch attached.
It seems this patch disables check for all cases when experimental is enabled, but check for overflow in only one case.
I am not sure I understand: Do you mean I missed a case where an overflow is now (after the patch) possible (but wasn't before) or do you mean there are formats after the patch that allow truncation and formats that do not allow it?
Ping. Carl Eugen
On Tue, Oct 11, 2016 at 10:06:54AM +0200, Carl Eugen Hoyos wrote:
2016-09-05 11:12 GMT+02:00 Carl Eugen Hoyos <ceffmpeg@gmail.com>:
2016-09-05 10:26 GMT+02:00 Paul B Mahol <onemda@gmail.com>:
On 9/5/16, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
New patch attached.
It seems this patch disables check for all cases when experimental is enabled, but check for overflow in only one case.
I am not sure I understand: Do you mean I missed a case where an overflow is now (after the patch) possible (but wasn't before) or do you mean there are formats after the patch that allow truncation and formats that do not allow it?
Ping.
i didnt look at the code but from the diff it seems what was meant was that bytestream + n could point outside the array that is indeed (suprising to many) undefined, you dont need to do bytestream[n] [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB it is not once nor twice but times without number that the same ideas make their appearance in the world. -- Aristotle
On Tue, Oct 11, 2016 at 11:16:48AM +0200, Michael Niedermayer wrote:
On Tue, Oct 11, 2016 at 10:06:54AM +0200, Carl Eugen Hoyos wrote:
2016-09-05 11:12 GMT+02:00 Carl Eugen Hoyos <ceffmpeg@gmail.com>:
2016-09-05 10:26 GMT+02:00 Paul B Mahol <onemda@gmail.com>:
On 9/5/16, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
New patch attached.
It seems this patch disables check for all cases when experimental is enabled, but check for overflow in only one case.
I am not sure I understand: Do you mean I missed a case where an overflow is now (after the patch) possible (but wasn't before) or do you mean there are formats after the patch that allow truncation and formats that do not allow it?
Ping.
i didnt look at the code but from the diff it seems what was meant was that bytestream + n could point outside the array that is indeed (suprising to many) undefined, you dont need to do bytestream[n]
i just stumbled across this again the correct way to check for the end (overflow wise) is if (n > s->bytestream_end - s->bytestream) ... also ptr[] should be memset (probably to 0) when there is no more input [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Dictatorship: All citizens are under surveillance, all their steps and actions recorded, for the politicians to enforce control. Democracy: All politicians are under surveillance, all their steps and actions recorded, for the citizens to enforce control.
participants (4)
-
Carl Eugen Hoyos -
Carl Eugen Hoyos -
Michael Niedermayer -
Paul B Mahol