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

[Michael Niedermayer]

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 at 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 at protonmail.com>
> >>> Signed-off-by: Michael Niedermayer <michael at 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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Those who are too smart to engage in politics are punished by being
governed by those who are dumber. -- Plato 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20200521/f3b47405/attachment.sig>