| Message ID | 20200219154951.30466-1-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/tiff: Check for Tiled and Stripped TIFFs | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
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.
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
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". >
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 [...]
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)
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 --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;
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(+)