diff mbox series

[FFmpeg-devel] avcodec/tiff: Check for Tiled and Stripped TIFFs

Message ID 20200219154951.30466-1-michael@niedermayer.cc
State New
Headers
Series [FFmpeg-devel] avcodec/tiff: Check for Tiled and Stripped TIFFs |

Checks

Context Check Description
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Michael Niedermayer Feb. 19, 2020, 3:49 p.m. UTC 
TIFF 6 spec: "Do not use both strip-oriented and tile-oriented fields in the same TIFF file."

Fixes: null pointer use, crash
Fixes: crash-762680f9d1b27f9b9085e12887ad44893fb2b020

Found-by: Shiziru <lunasl@protonmail.com>
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/tiff.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Anton Khirnov Feb. 27, 2020, 2:05 p.m. UTC | #1 
Quoting Michael Niedermayer (2020-02-19 16:49:51)
> TIFF 6 spec: "Do not use both strip-oriented and tile-oriented fields in the same TIFF file."
> 
> Fixes: null pointer use, crash
> Fixes: crash-762680f9d1b27f9b9085e12887ad44893fb2b020
> 
> Found-by: Shiziru <lunasl@protonmail.com>
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/tiff.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/libavcodec/tiff.c b/libavcodec/tiff.c
> index e8357114de..fd50b1cbfa 100644
> --- a/libavcodec/tiff.c
> +++ b/libavcodec/tiff.c
> @@ -1873,6 +1873,12 @@ again:
>          return AVERROR_INVALIDDATA;
>      }
>  
> +    if (   (s->is_tiled || s->tile_byte_counts_offset || s->tile_offsets_offset || s->tile_width || s->tile_length || s->tile_count)
> +        && (s->strippos || s->strips || s->stripoff || s->rps || s->sot || s->sstype || s->stripsize || s->stripsizesoff)) {

This is horribly unreadable. Putting those checks into macros, like
HAVE_TILES(s) and HAVE_STRIPS(s) would make it a lot better.
Carl Eugen Hoyos Feb. 27, 2020, 2:10 p.m. UTC | #2 
Am Do., 27. Feb. 2020 um 15:05 Uhr schrieb Anton Khirnov <anton@khirnov.net>:
>
> Quoting Michael Niedermayer (2020-02-19 16:49:51)
> > TIFF 6 spec: "Do not use both strip-oriented and tile-oriented fields in the same TIFF file."
> >
> > Fixes: null pointer use, crash
> > Fixes: crash-762680f9d1b27f9b9085e12887ad44893fb2b020
> >
> > Found-by: Shiziru <lunasl@protonmail.com>
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/tiff.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/libavcodec/tiff.c b/libavcodec/tiff.c
> > index e8357114de..fd50b1cbfa 100644
> > --- a/libavcodec/tiff.c
> > +++ b/libavcodec/tiff.c
> > @@ -1873,6 +1873,12 @@ again:
> >          return AVERROR_INVALIDDATA;
> >      }
> >
> > +    if (   (s->is_tiled || s->tile_byte_counts_offset || s->tile_offsets_offset || s->tile_width || s->tile_length || s->tile_count)
> > +        && (s->strippos || s->strips || s->stripoff || s->rps || s->sot || s->sstype || s->stripsize || s->stripsizesoff)) {
>
> This is horribly unreadable. Putting those checks into macros, like
> HAVE_TILES(s) and HAVE_STRIPS(s) would make it a lot better.

Were else in the file could the macros be used?
I fear that adding macros that are only used in one place will make the
code much more unreadable.

Carl Eugen
James Almer Feb. 27, 2020, 3:07 p.m. UTC | #3 
On 2/27/2020 11:10 AM, Carl Eugen Hoyos wrote:
> Am Do., 27. Feb. 2020 um 15:05 Uhr schrieb Anton Khirnov <anton@khirnov.net>:
>>
>> Quoting Michael Niedermayer (2020-02-19 16:49:51)
>>> TIFF 6 spec: "Do not use both strip-oriented and tile-oriented fields in the same TIFF file."
>>>
>>> Fixes: null pointer use, crash
>>> Fixes: crash-762680f9d1b27f9b9085e12887ad44893fb2b020
>>>
>>> Found-by: Shiziru <lunasl@protonmail.com>
>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>> ---
>>>  libavcodec/tiff.c | 6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/libavcodec/tiff.c b/libavcodec/tiff.c
>>> index e8357114de..fd50b1cbfa 100644
>>> --- a/libavcodec/tiff.c
>>> +++ b/libavcodec/tiff.c
>>> @@ -1873,6 +1873,12 @@ again:
>>>          return AVERROR_INVALIDDATA;
>>>      }
>>>
>>> +    if (   (s->is_tiled || s->tile_byte_counts_offset || s->tile_offsets_offset || s->tile_width || s->tile_length || s->tile_count)
>>> +        && (s->strippos || s->strips || s->stripoff || s->rps || s->sot || s->sstype || s->stripsize || s->stripsizesoff)) {
>>
>> This is horribly unreadable. Putting those checks into macros, like
>> HAVE_TILES(s) and HAVE_STRIPS(s) would make it a lot better.
> 
> Were else in the file could the macros be used?
> I fear that adding macros that are only used in one place will make the
> code much more unreadable.

I don't agree. An "if (HAVE_TILES(s) && HAVE_STRIPS(s))" check is easy
to understand at first glance, regardless of the internals of each of
those macros. It achieves the same effect as adding a comment in the
code to explain what all those checks do.

See IS_IRAP_NAL(nal) and IS_IDR_NAL(nal) in hevc_parser for another
example of this simplification, which are also used in a single place.

> 
> Carl Eugen
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
Michael Niedermayer May 21, 2020, 6:59 a.m. UTC | #4 
On Thu, Feb 27, 2020 at 12:07:16PM -0300, James Almer wrote:
> On 2/27/2020 11:10 AM, Carl Eugen Hoyos wrote:
> > Am Do., 27. Feb. 2020 um 15:05 Uhr schrieb Anton Khirnov <anton@khirnov.net>:
> >>
> >> Quoting Michael Niedermayer (2020-02-19 16:49:51)
> >>> TIFF 6 spec: "Do not use both strip-oriented and tile-oriented fields in the same TIFF file."
> >>>
> >>> Fixes: null pointer use, crash
> >>> Fixes: crash-762680f9d1b27f9b9085e12887ad44893fb2b020
> >>>
> >>> Found-by: Shiziru <lunasl@protonmail.com>
> >>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> >>> ---
> >>>  libavcodec/tiff.c | 6 ++++++
> >>>  1 file changed, 6 insertions(+)
> >>>
> >>> diff --git a/libavcodec/tiff.c b/libavcodec/tiff.c
> >>> index e8357114de..fd50b1cbfa 100644
> >>> --- a/libavcodec/tiff.c
> >>> +++ b/libavcodec/tiff.c
> >>> @@ -1873,6 +1873,12 @@ again:
> >>>          return AVERROR_INVALIDDATA;
> >>>      }
> >>>
> >>> +    if (   (s->is_tiled || s->tile_byte_counts_offset || s->tile_offsets_offset || s->tile_width || s->tile_length || s->tile_count)
> >>> +        && (s->strippos || s->strips || s->stripoff || s->rps || s->sot || s->sstype || s->stripsize || s->stripsizesoff)) {
> >>
> >> This is horribly unreadable. Putting those checks into macros, like
> >> HAVE_TILES(s) and HAVE_STRIPS(s) would make it a lot better.
> > 
> > Were else in the file could the macros be used?
> > I fear that adding macros that are only used in one place will make the
> > code much more unreadable.
> 
> I don't agree. An "if (HAVE_TILES(s) && HAVE_STRIPS(s))" check is easy
> to understand at first glance, regardless of the internals of each of
> those macros. It achieves the same effect as adding a comment in the
> code to explain what all those checks do.
> 

> See IS_IRAP_NAL(nal) and IS_IDR_NAL(nal) in hevc_parser for another
> example of this simplification, which are also used in a single place.

I dont see IS_IRAP_NAL() and IS_IDR_NAL() to check every field in the headers
to be consistent with IDR / IRAP NALs, which is what the tiff code in this
thread does kind off ..

to match hevc, i could add something like
#define HAVE_TILES(s)  s->has_tiles
#define HAVE_STRIPS(s) s->strips

but iam not sure what that would simplify
The validity checks are needed once and if thats put in a macro then maybe that
could be called HAVE_RANDOM_TILE_BITS() and HAVE_RANDOM_STRIP_BITS() could be
maybe a possibility
then again if thats what is suggested then i would suggest to rather add 2
local variable with these names instead of a macro which would seem more
readable

whats your opinion ?


Thanks

[...]
Anton Khirnov May 21, 2020, 10:13 a.m. UTC | #5 
Quoting Michael Niedermayer (2020-05-21 08:59:27)
> 
> I dont see IS_IRAP_NAL() and IS_IDR_NAL() to check every field in the headers
> to be consistent with IDR / IRAP NALs, which is what the tiff code in this
> thread does kind off ..
> 
> to match hevc, i could add something like
> #define HAVE_TILES(s)  s->has_tiles
> #define HAVE_STRIPS(s) s->strips
> 
> but iam not sure what that would simplify
> The validity checks are needed once and if thats put in a macro then maybe that
> could be called HAVE_RANDOM_TILE_BITS() and HAVE_RANDOM_STRIP_BITS() could be
> maybe a possibility
> then again if thats what is suggested then i would suggest to rather add 2
> local variable with these names instead of a macro which would seem more
> readable
> 
> whats your opinion ?

I see little difference between a macro and a local variable in this
case, either one is fine with me. And both are significantly more
readable than a giant condition that the reader has to reverse engineer
into (have_tiles && have_strips)
Michael Niedermayer May 21, 2020, 12:17 p.m. UTC | #6 
On Thu, May 21, 2020 at 12:13:15PM +0200, Anton Khirnov wrote:
> Quoting Michael Niedermayer (2020-05-21 08:59:27)
> > 
> > I dont see IS_IRAP_NAL() and IS_IDR_NAL() to check every field in the headers
> > to be consistent with IDR / IRAP NALs, which is what the tiff code in this
> > thread does kind off ..
> > 
> > to match hevc, i could add something like
> > #define HAVE_TILES(s)  s->has_tiles
> > #define HAVE_STRIPS(s) s->strips
> > 
> > but iam not sure what that would simplify
> > The validity checks are needed once and if thats put in a macro then maybe that
> > could be called HAVE_RANDOM_TILE_BITS() and HAVE_RANDOM_STRIP_BITS() could be
> > maybe a possibility
> > then again if thats what is suggested then i would suggest to rather add 2
> > local variable with these names instead of a macro which would seem more
> > readable
> > 
> > whats your opinion ?
> 
> I see little difference between a macro and a local variable in this
> case, either one is fine with me. And both are significantly more
> readable than a giant condition that the reader has to reverse engineer
> into (have_tiles && have_strips)

ok, will apply it with local variables

thanks

[...]
diff mbox series

Patch

diff --git a/libavcodec/tiff.c b/libavcodec/tiff.c
index e8357114de..fd50b1cbfa 100644
--- a/libavcodec/tiff.c
+++ b/libavcodec/tiff.c
@@ -1873,6 +1873,12 @@  again:
         return AVERROR_INVALIDDATA;
     }
 
+    if (   (s->is_tiled || s->tile_byte_counts_offset || s->tile_offsets_offset || s->tile_width || s->tile_length || s->tile_count)
+        && (s->strippos || s->strips || s->stripoff || s->rps || s->sot || s->sstype || s->stripsize || s->stripsizesoff)) {
+        av_log(avctx, AV_LOG_ERROR, "Tiled TIFF is not allowed to strip\n");
+        return AVERROR_INVALIDDATA;
+    }
+
     /* now we have the data and may start decoding */
     if ((ret = init_image(s, &frame)) < 0)
         return ret;