[PATCH 1/7] avcodec/av1dec: bit_depth cannot be another values than 8, 10, 12
Fixes: CID1544265 Logically dead code Sponsored-by: Sovereign Tech Fund Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/av1dec.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/libavcodec/av1dec.c b/libavcodec/av1dec.c index 79a30a114da..4f9222cca27 100644 --- a/libavcodec/av1dec.c +++ b/libavcodec/av1dec.c @@ -493,7 +493,7 @@ static enum AVPixelFormat get_sw_pixel_format(void *logctx, else if (bit_depth == 12) pix_fmt = AV_PIX_FMT_YUV444P12; else - av_log(logctx, AV_LOG_WARNING, "Unknown AV1 pixel format.\n"); + av_assert0(0); } else if (seq->color_config.subsampling_x == 1 && seq->color_config.subsampling_y == 0) { if (bit_depth == 8) @@ -503,7 +503,7 @@ static enum AVPixelFormat get_sw_pixel_format(void *logctx, else if (bit_depth == 12) pix_fmt = AV_PIX_FMT_YUV422P12; else - av_log(logctx, AV_LOG_WARNING, "Unknown AV1 pixel format.\n"); + av_assert0(0); } else if (seq->color_config.subsampling_x == 1 && seq->color_config.subsampling_y == 1) { if (bit_depth == 8) @@ -513,7 +513,7 @@ static enum AVPixelFormat get_sw_pixel_format(void *logctx, else if (bit_depth == 12) pix_fmt = AV_PIX_FMT_YUV420P12; else - av_log(logctx, AV_LOG_WARNING, "Unknown AV1 pixel format.\n"); + av_assert0(0); } } else { if (bit_depth == 8) @@ -523,7 +523,7 @@ static enum AVPixelFormat get_sw_pixel_format(void *logctx, else if (bit_depth == 12) pix_fmt = AV_PIX_FMT_GRAY12; else - av_log(logctx, AV_LOG_WARNING, "Unknown AV1 pixel format.\n"); + av_assert0(0); } return pix_fmt; -- 2.43.2
Fixes: CID1596605 Uninitialized scalar variable Sponsored-by: Sovereign Tech Fund Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/av1dec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libavcodec/av1dec.c b/libavcodec/av1dec.c index 4f9222cca27..93ab04eb378 100644 --- a/libavcodec/av1dec.c +++ b/libavcodec/av1dec.c @@ -1262,7 +1262,7 @@ static int av1_receive_frame_internal(AVCodecContext *avctx, AVFrame *frame) { AV1DecContext *s = avctx->priv_data; AV1RawTileGroup *raw_tile_group = NULL; - int i = 0, ret; + int i = 0, ret = 0; for (i = s->nb_unit; i < s->current_obu.nb_units; i++) { CodedBitstreamUnit *unit = &s->current_obu.units[i]; -- 2.43.2
On 5/1/2024 9:41 PM, Michael Niedermayer wrote:
Fixes: CID1596605 Uninitialized scalar variable
Sponsored-by: Sovereign Tech Fund Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/av1dec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libavcodec/av1dec.c b/libavcodec/av1dec.c index 4f9222cca27..93ab04eb378 100644 --- a/libavcodec/av1dec.c +++ b/libavcodec/av1dec.c @@ -1262,7 +1262,7 @@ static int av1_receive_frame_internal(AVCodecContext *avctx, AVFrame *frame) { AV1DecContext *s = avctx->priv_data; AV1RawTileGroup *raw_tile_group = NULL; - int i = 0, ret; + int i = 0, ret = 0;
for (i = s->nb_unit; i < s->current_obu.nb_units; i++) { CodedBitstreamUnit *unit = &s->current_obu.units[i];
Should be ok. Alternatively, we could set ret to AVERROR_BUG here and to 0 in the places where it's missing (and that triggered coverity).
Michael Niedermayer:
Fixes: CID1596605 Uninitialized scalar variable
Sponsored-by: Sovereign Tech Fund Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/av1dec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libavcodec/av1dec.c b/libavcodec/av1dec.c index 4f9222cca27..93ab04eb378 100644 --- a/libavcodec/av1dec.c +++ b/libavcodec/av1dec.c @@ -1262,7 +1262,7 @@ static int av1_receive_frame_internal(AVCodecContext *avctx, AVFrame *frame) { AV1DecContext *s = avctx->priv_data; AV1RawTileGroup *raw_tile_group = NULL; - int i = 0, ret; + int i = 0, ret = 0;
for (i = s->nb_unit; i < s->current_obu.nb_units; i++) { CodedBitstreamUnit *unit = &s->current_obu.units[i];
A better approach is to actually initialize ret before every goto end in order to ensure that only the actually desired ret is returned and not some earlier value. - Andreas
On Thu, May 02, 2024 at 09:12:36AM +0200, Andreas Rheinhardt wrote:
Michael Niedermayer:
Fixes: CID1596605 Uninitialized scalar variable
Sponsored-by: Sovereign Tech Fund Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/av1dec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libavcodec/av1dec.c b/libavcodec/av1dec.c index 4f9222cca27..93ab04eb378 100644 --- a/libavcodec/av1dec.c +++ b/libavcodec/av1dec.c @@ -1262,7 +1262,7 @@ static int av1_receive_frame_internal(AVCodecContext *avctx, AVFrame *frame) { AV1DecContext *s = avctx->priv_data; AV1RawTileGroup *raw_tile_group = NULL; - int i = 0, ret; + int i = 0, ret = 0;
for (i = s->nb_unit; i < s->current_obu.nb_units; i++) { CodedBitstreamUnit *unit = &s->current_obu.units[i];
A better approach is to actually initialize ret before every goto end in order to ensure that only the actually desired ret is returned and not some earlier value.
ok, will apply with ret = 0 before the 2 goto end lacking it that i see 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 ..."
Michael Niedermayer:
On Thu, May 02, 2024 at 09:12:36AM +0200, Andreas Rheinhardt wrote:
Michael Niedermayer:
Fixes: CID1596605 Uninitialized scalar variable
Sponsored-by: Sovereign Tech Fund Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/av1dec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libavcodec/av1dec.c b/libavcodec/av1dec.c index 4f9222cca27..93ab04eb378 100644 --- a/libavcodec/av1dec.c +++ b/libavcodec/av1dec.c @@ -1262,7 +1262,7 @@ static int av1_receive_frame_internal(AVCodecContext *avctx, AVFrame *frame) { AV1DecContext *s = avctx->priv_data; AV1RawTileGroup *raw_tile_group = NULL; - int i = 0, ret; + int i = 0, ret = 0;
for (i = s->nb_unit; i < s->current_obu.nb_units; i++) { CodedBitstreamUnit *unit = &s->current_obu.units[i];
A better approach is to actually initialize ret before every goto end in order to ensure that only the actually desired ret is returned and not some earlier value.
ok, will apply with ret = 0 before the 2 goto end lacking it that i see
I already sent a patch for this. - Andreas
On Thu, May 02, 2024 at 11:34:48PM +0200, Andreas Rheinhardt wrote:
Michael Niedermayer:
On Thu, May 02, 2024 at 09:12:36AM +0200, Andreas Rheinhardt wrote:
Michael Niedermayer:
Fixes: CID1596605 Uninitialized scalar variable
Sponsored-by: Sovereign Tech Fund Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/av1dec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libavcodec/av1dec.c b/libavcodec/av1dec.c index 4f9222cca27..93ab04eb378 100644 --- a/libavcodec/av1dec.c +++ b/libavcodec/av1dec.c @@ -1262,7 +1262,7 @@ static int av1_receive_frame_internal(AVCodecContext *avctx, AVFrame *frame) { AV1DecContext *s = avctx->priv_data; AV1RawTileGroup *raw_tile_group = NULL; - int i = 0, ret; + int i = 0, ret = 0;
for (i = s->nb_unit; i < s->current_obu.nb_units; i++) { CodedBitstreamUnit *unit = &s->current_obu.units[i];
A better approach is to actually initialize ret before every goto end in order to ensure that only the actually desired ret is returned and not some earlier value.
ok, will apply with ret = 0 before the 2 goto end lacking it that i see
I already sent a patch for this.
ok, ill drop mine then thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The bravest are surely those who have the clearest vision of what is before them, glory and danger alike, and yet notwithstanding go out to meet it. -- Thucydides
Fixes: CID1543204 Logically dead code Sponsored-by: Sovereign Tech Fund Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/avfft.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libavcodec/avfft.c b/libavcodec/avfft.c index f6787937f67..0f43f30b776 100644 --- a/libavcodec/avfft.c +++ b/libavcodec/avfft.c @@ -158,7 +158,7 @@ RDFTContext *av_rdft_init(int nbits, enum RDFTransformType trans) return NULL; } - s->stride = (trans == DFT_C2R) ? sizeof(AVComplexFloat) : sizeof(float); + s->stride = sizeof(float); s->len = 1 << nbits; s->inv = trans == IDFT_C2R; -- 2.43.2
May 2, 2024, 02:42 by michael@niedermayer.cc:
Fixes: CID1543204 Logically dead code
Sponsored-by: Sovereign Tech Fund Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/avfft.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libavcodec/avfft.c b/libavcodec/avfft.c index f6787937f67..0f43f30b776 100644 --- a/libavcodec/avfft.c +++ b/libavcodec/avfft.c @@ -158,7 +158,7 @@ RDFTContext *av_rdft_init(int nbits, enum RDFTransformType trans) return NULL; }
- s->stride = (trans == DFT_C2R) ? sizeof(AVComplexFloat) : sizeof(float); + s->stride = sizeof(float); s->len = 1 << nbits; s->inv = trans == IDFT_C2R;
That's not right. While it's true that currently the stride parameter in av_tx_fn is unused for RDFTs, that may not always be the case, and the documentation requires that the stride is valid and set to the value that the current implementation assumes, so that nothing breaks once that is implemented.
On Thu, May 02, 2024 at 03:21:11AM +0200, Lynne wrote:
May 2, 2024, 02:42 by michael@niedermayer.cc:
Fixes: CID1543204 Logically dead code
Sponsored-by: Sovereign Tech Fund Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/avfft.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libavcodec/avfft.c b/libavcodec/avfft.c index f6787937f67..0f43f30b776 100644 --- a/libavcodec/avfft.c +++ b/libavcodec/avfft.c @@ -158,7 +158,7 @@ RDFTContext *av_rdft_init(int nbits, enum RDFTransformType trans) return NULL; }
- s->stride = (trans == DFT_C2R) ? sizeof(AVComplexFloat) : sizeof(float); + s->stride = sizeof(float); s->len = 1 << nbits; s->inv = trans == IDFT_C2R;
That's not right. While it's true that currently the stride parameter in av_tx_fn is unused for RDFTs, that may not always be the case, and the documentation requires that the stride is valid and set to the value that the current implementation assumes, so that nothing breaks once that is implemented.
i kind of had the same feeling but wanted to post it anyway. so patch withdrawn, ill mark the issue as "intentional" in coverity thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Never trust a computer, one day, it may think you are the virus. -- Compn
Fixes: CID1506708 Unchecked return value Sponsored-by: Sovereign Tech Fund Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/avs2_parser.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/libavcodec/avs2_parser.c b/libavcodec/avs2_parser.c index 200134f91db..8d4bc3cee0d 100644 --- a/libavcodec/avs2_parser.c +++ b/libavcodec/avs2_parser.c @@ -72,13 +72,15 @@ static void parse_avs2_seq_header(AVCodecParserContext *s, const uint8_t *buf, unsigned aspect_ratio; unsigned frame_rate_code; int low_delay; + int ret; // update buf_size_min if parse more deeper const int buf_size_min = 15; if (buf_size < buf_size_min) return; - init_get_bits8(&gb, buf, buf_size_min); + ret = init_get_bits8(&gb, buf, buf_size_min); + av_assert2(ret >= 0); s->key_frame = 1; s->pict_type = AV_PICTURE_TYPE_I; -- 2.43.2
Michael Niedermayer:
Fixes: CID1506708 Unchecked return value
Sponsored-by: Sovereign Tech Fund Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/avs2_parser.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/libavcodec/avs2_parser.c b/libavcodec/avs2_parser.c index 200134f91db..8d4bc3cee0d 100644 --- a/libavcodec/avs2_parser.c +++ b/libavcodec/avs2_parser.c @@ -72,13 +72,15 @@ static void parse_avs2_seq_header(AVCodecParserContext *s, const uint8_t *buf, unsigned aspect_ratio; unsigned frame_rate_code; int low_delay; + int ret; // update buf_size_min if parse more deeper const int buf_size_min = 15;
if (buf_size < buf_size_min) return;
- init_get_bits8(&gb, buf, buf_size_min); + ret = init_get_bits8(&gb, buf, buf_size_min); + av_assert2(ret >= 0);
s->key_frame = 1; s->pict_type = AV_PICTURE_TYPE_I;
Code like this may trigger set-but-unused variable warnings when ASSERT_LEVEL is <= 1. Add av_unused to ret to fix this. Apart from that, it should be av_assert1 (this is not hot). - Andreas
On Thu, May 02, 2024 at 10:25:24AM +0200, Andreas Rheinhardt wrote:
Michael Niedermayer:
Fixes: CID1506708 Unchecked return value
Sponsored-by: Sovereign Tech Fund Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/avs2_parser.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/libavcodec/avs2_parser.c b/libavcodec/avs2_parser.c index 200134f91db..8d4bc3cee0d 100644 --- a/libavcodec/avs2_parser.c +++ b/libavcodec/avs2_parser.c @@ -72,13 +72,15 @@ static void parse_avs2_seq_header(AVCodecParserContext *s, const uint8_t *buf, unsigned aspect_ratio; unsigned frame_rate_code; int low_delay; + int ret; // update buf_size_min if parse more deeper const int buf_size_min = 15;
if (buf_size < buf_size_min) return;
- init_get_bits8(&gb, buf, buf_size_min); + ret = init_get_bits8(&gb, buf, buf_size_min); + av_assert2(ret >= 0);
s->key_frame = 1; s->pict_type = AV_PICTURE_TYPE_I;
Code like this may trigger set-but-unused variable warnings when ASSERT_LEVEL is <= 1. Add av_unused to ret to fix this. Apart from that, it should be av_assert1 (this is not hot).
will apply with these changes thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Observe your enemies, for they first find out your faults. -- Antisthenes
Fixes: CID1492867 Unchecked return value Sponsored-by: Sovereign Tech Fund Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/avs3_parser.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/libavcodec/avs3_parser.c b/libavcodec/avs3_parser.c index a819b5783d6..0f9076befe1 100644 --- a/libavcodec/avs3_parser.c +++ b/libavcodec/avs3_parser.c @@ -73,7 +73,9 @@ static void parse_avs3_nal_units(AVCodecParserContext *s, const uint8_t *buf, GetBitContext gb; int profile, ratecode, low_delay; - init_get_bits8(&gb, buf + 4, buf_size - 4); + int ret = init_get_bits8(&gb, buf + 4, buf_size - 4); + if (ret < 0) + return; s->key_frame = 1; s->pict_type = AV_PICTURE_TYPE_I; -- 2.43.2
Michael Niedermayer:
Fixes: CID1492867 Unchecked return value
Sponsored-by: Sovereign Tech Fund Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/avs3_parser.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/libavcodec/avs3_parser.c b/libavcodec/avs3_parser.c index a819b5783d6..0f9076befe1 100644 --- a/libavcodec/avs3_parser.c +++ b/libavcodec/avs3_parser.c @@ -73,7 +73,9 @@ static void parse_avs3_nal_units(AVCodecParserContext *s, const uint8_t *buf, GetBitContext gb; int profile, ratecode, low_delay;
- init_get_bits8(&gb, buf + 4, buf_size - 4); + int ret = init_get_bits8(&gb, buf + 4, buf_size - 4); + if (ret < 0) + return;
s->key_frame = 1; s->pict_type = AV_PICTURE_TYPE_I;
This code only reads/skips a few bits here (at most 100 if I counted correctly), so one could initialize the reader for a shorter length and assert that the call succeeds. - Andreas
On Thu, May 02, 2024 at 10:28:36AM +0200, Andreas Rheinhardt wrote:
Michael Niedermayer:
Fixes: CID1492867 Unchecked return value
Sponsored-by: Sovereign Tech Fund Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/avs3_parser.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/libavcodec/avs3_parser.c b/libavcodec/avs3_parser.c index a819b5783d6..0f9076befe1 100644 --- a/libavcodec/avs3_parser.c +++ b/libavcodec/avs3_parser.c @@ -73,7 +73,9 @@ static void parse_avs3_nal_units(AVCodecParserContext *s, const uint8_t *buf, GetBitContext gb; int profile, ratecode, low_delay;
- init_get_bits8(&gb, buf + 4, buf_size - 4); + int ret = init_get_bits8(&gb, buf + 4, buf_size - 4); + if (ret < 0) + return;
s->key_frame = 1; s->pict_type = AV_PICTURE_TYPE_I;
This code only reads/skips a few bits here (at most 100 if I counted correctly), so one could initialize the reader for a shorter length and assert that the call succeeds.
will apply: - init_get_bits8(&gb, buf + 4, buf_size - 4); + av_unused int ret = init_get_bits(&gb, buf + 4, 100); + av_assert1(ret >= 0); thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The educated differ from the uneducated as much as the living from the dead. -- Aristotle
Fixes: CID1465488 Unintentional integer overflow Sponsored-by: Sovereign Tech Fund Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/cbs_av1.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libavcodec/cbs_av1.c b/libavcodec/cbs_av1.c index 1d9ac5ab449..fb829960220 100644 --- a/libavcodec/cbs_av1.c +++ b/libavcodec/cbs_av1.c @@ -301,7 +301,7 @@ static int cbs_av1_write_increment(CodedBitstreamContext *ctx, PutBitContext *pb return AVERROR(ENOSPC); if (len > 0) - put_bits(pbc, len, (1 << len) - 1 - (value != range_max)); + put_bits(pbc, len, (1U << len) - 1 - (value != range_max)); CBS_TRACE_WRITE_END_NO_SUBSCRIPTS(); -- 2.43.2
On Thu, May 02, 2024 at 02:41:49AM +0200, Michael Niedermayer wrote:
Fixes: CID1465488 Unintentional integer overflow
Sponsored-by: Sovereign Tech Fund Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/cbs_av1.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
will apply [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB What does censorship reveal? It reveals fear. -- Julian Assange
Fixes: CID1439654 Untrusted pointer read Sponsored-by: Sovereign Tech Fund Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/cbs_jpeg.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libavcodec/cbs_jpeg.c b/libavcodec/cbs_jpeg.c index b1b58dcd65e..406147c082c 100644 --- a/libavcodec/cbs_jpeg.c +++ b/libavcodec/cbs_jpeg.c @@ -146,13 +146,13 @@ static int cbs_jpeg_split_fragment(CodedBitstreamContext *ctx, } } else { i = start; - if (i + 2 > frag->data_size) { + if (i > frag->data_size - 2) { av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid JPEG image: " "truncated at %02x marker.\n", marker); return AVERROR_INVALIDDATA; } length = AV_RB16(frag->data + i); - if (i + length > frag->data_size) { + if (length > frag->data_size - i) { av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid JPEG image: " "truncated at %02x marker segment.\n", marker); return AVERROR_INVALIDDATA; -- 2.43.2
Michael Niedermayer:
Fixes: CID1439654 Untrusted pointer read
Sponsored-by: Sovereign Tech Fund Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/cbs_jpeg.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/libavcodec/cbs_jpeg.c b/libavcodec/cbs_jpeg.c index b1b58dcd65e..406147c082c 100644 --- a/libavcodec/cbs_jpeg.c +++ b/libavcodec/cbs_jpeg.c @@ -146,13 +146,13 @@ static int cbs_jpeg_split_fragment(CodedBitstreamContext *ctx, } } else { i = start; - if (i + 2 > frag->data_size) { + if (i > frag->data_size - 2) { av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid JPEG image: " "truncated at %02x marker.\n", marker); return AVERROR_INVALIDDATA; } length = AV_RB16(frag->data + i); - if (i + length > frag->data_size) { + if (length > frag->data_size - i) { av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid JPEG image: " "truncated at %02x marker segment.\n", marker); return AVERROR_INVALIDDATA;
You should always mention when you are not fixing bugs in our code, but rather intend to apply workaround for coverity crazyness (i.e. the requirement that reading values in non-native endianness needs to be sanitized). - Andreas
On Thu, May 02, 2024 at 08:58:17AM +0200, Andreas Rheinhardt wrote:
Michael Niedermayer:
Fixes: CID1439654 Untrusted pointer read
Sponsored-by: Sovereign Tech Fund Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/cbs_jpeg.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/libavcodec/cbs_jpeg.c b/libavcodec/cbs_jpeg.c index b1b58dcd65e..406147c082c 100644 --- a/libavcodec/cbs_jpeg.c +++ b/libavcodec/cbs_jpeg.c @@ -146,13 +146,13 @@ static int cbs_jpeg_split_fragment(CodedBitstreamContext *ctx, } } else { i = start; - if (i + 2 > frag->data_size) { + if (i > frag->data_size - 2) { av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid JPEG image: " "truncated at %02x marker.\n", marker); return AVERROR_INVALIDDATA; } length = AV_RB16(frag->data + i); - if (i + length > frag->data_size) { + if (length > frag->data_size - i) { av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid JPEG image: " "truncated at %02x marker segment.\n", marker); return AVERROR_INVALIDDATA;
You should always mention when you are not fixing bugs in our code, but rather intend to apply workaround for coverity crazyness (i.e. the requirement that reading values in non-native endianness needs to be sanitized).
writing code like if (i + length > frag->data_size) is IMHO not proper and that has nothing to do with coverity the reason why this is not proper is that i + length could in principle overflow. It depends on teh whole loop and calling code if that is possible or not. to check length, length should be alone on one side of the check thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB It is what and why we do it that matters, not just one of them.
On Fri, May 03, 2024 at 12:03:57AM +0200, Michael Niedermayer wrote:
On Thu, May 02, 2024 at 08:58:17AM +0200, Andreas Rheinhardt wrote:
Michael Niedermayer:
Fixes: CID1439654 Untrusted pointer read
Sponsored-by: Sovereign Tech Fund Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/cbs_jpeg.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/libavcodec/cbs_jpeg.c b/libavcodec/cbs_jpeg.c index b1b58dcd65e..406147c082c 100644 --- a/libavcodec/cbs_jpeg.c +++ b/libavcodec/cbs_jpeg.c @@ -146,13 +146,13 @@ static int cbs_jpeg_split_fragment(CodedBitstreamContext *ctx, } } else { i = start; - if (i + 2 > frag->data_size) { + if (i > frag->data_size - 2) { av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid JPEG image: " "truncated at %02x marker.\n", marker); return AVERROR_INVALIDDATA; } length = AV_RB16(frag->data + i); - if (i + length > frag->data_size) { + if (length > frag->data_size - i) { av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid JPEG image: " "truncated at %02x marker segment.\n", marker); return AVERROR_INVALIDDATA;
You should always mention when you are not fixing bugs in our code, but rather intend to apply workaround for coverity crazyness (i.e. the requirement that reading values in non-native endianness needs to be sanitized).
writing code like if (i + length > frag->data_size)
is IMHO not proper and that has nothing to do with coverity the reason why this is not proper is that i + length could in principle overflow. It depends on teh whole loop and calling code if that is possible or not.
to check length, length should be alone on one side of the check
I will add this to the commit message: " The checked entity should be alone on one side of the check, this avoids complex considerations of overflows. This fixes a issue of bad style in our code and a coverity issue. " thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Complexity theory is the science of finding the exact solution to an approximation. Benchmarking OTOH is finding an approximation of the exact
On 5/1/2024 9:41 PM, Michael Niedermayer wrote:
Fixes: CID1544265 Logically dead code
Sponsored-by: Sovereign Tech Fund Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/av1dec.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/libavcodec/av1dec.c b/libavcodec/av1dec.c index 79a30a114da..4f9222cca27 100644 --- a/libavcodec/av1dec.c +++ b/libavcodec/av1dec.c @@ -493,7 +493,7 @@ static enum AVPixelFormat get_sw_pixel_format(void *logctx, else if (bit_depth == 12) pix_fmt = AV_PIX_FMT_YUV444P12; else - av_log(logctx, AV_LOG_WARNING, "Unknown AV1 pixel format.\n"); + av_assert0(0); } else if (seq->color_config.subsampling_x == 1 && seq->color_config.subsampling_y == 0) { if (bit_depth == 8) @@ -503,7 +503,7 @@ static enum AVPixelFormat get_sw_pixel_format(void *logctx, else if (bit_depth == 12) pix_fmt = AV_PIX_FMT_YUV422P12; else - av_log(logctx, AV_LOG_WARNING, "Unknown AV1 pixel format.\n"); + av_assert0(0); } else if (seq->color_config.subsampling_x == 1 && seq->color_config.subsampling_y == 1) { if (bit_depth == 8) @@ -513,7 +513,7 @@ static enum AVPixelFormat get_sw_pixel_format(void *logctx, else if (bit_depth == 12) pix_fmt = AV_PIX_FMT_YUV420P12; else - av_log(logctx, AV_LOG_WARNING, "Unknown AV1 pixel format.\n"); + av_assert0(0); } } else { if (bit_depth == 8) @@ -523,7 +523,7 @@ static enum AVPixelFormat get_sw_pixel_format(void *logctx, else if (bit_depth == 12) pix_fmt = AV_PIX_FMT_GRAY12; else - av_log(logctx, AV_LOG_WARNING, "Unknown AV1 pixel format.\n"); + av_assert0(0); }
return pix_fmt;
LGTM. Can you change bit_depth into an int while at it? no reason for it to be uint8_t as a scalar.
On Wed, May 01, 2024 at 09:56:53PM -0300, James Almer wrote:
On 5/1/2024 9:41 PM, Michael Niedermayer wrote:
Fixes: CID1544265 Logically dead code
Sponsored-by: Sovereign Tech Fund Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/av1dec.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/libavcodec/av1dec.c b/libavcodec/av1dec.c index 79a30a114da..4f9222cca27 100644 --- a/libavcodec/av1dec.c +++ b/libavcodec/av1dec.c @@ -493,7 +493,7 @@ static enum AVPixelFormat get_sw_pixel_format(void *logctx, else if (bit_depth == 12) pix_fmt = AV_PIX_FMT_YUV444P12; else - av_log(logctx, AV_LOG_WARNING, "Unknown AV1 pixel format.\n"); + av_assert0(0); } else if (seq->color_config.subsampling_x == 1 && seq->color_config.subsampling_y == 0) { if (bit_depth == 8) @@ -503,7 +503,7 @@ static enum AVPixelFormat get_sw_pixel_format(void *logctx, else if (bit_depth == 12) pix_fmt = AV_PIX_FMT_YUV422P12; else - av_log(logctx, AV_LOG_WARNING, "Unknown AV1 pixel format.\n"); + av_assert0(0); } else if (seq->color_config.subsampling_x == 1 && seq->color_config.subsampling_y == 1) { if (bit_depth == 8) @@ -513,7 +513,7 @@ static enum AVPixelFormat get_sw_pixel_format(void *logctx, else if (bit_depth == 12) pix_fmt = AV_PIX_FMT_YUV420P12; else - av_log(logctx, AV_LOG_WARNING, "Unknown AV1 pixel format.\n"); + av_assert0(0); } } else { if (bit_depth == 8) @@ -523,7 +523,7 @@ static enum AVPixelFormat get_sw_pixel_format(void *logctx, else if (bit_depth == 12) pix_fmt = AV_PIX_FMT_GRAY12; else - av_log(logctx, AV_LOG_WARNING, "Unknown AV1 pixel format.\n"); + av_assert0(0); } return pix_fmt;
LGTM.
will apply
Can you change bit_depth into an int while at it? no reason for it to be uint8_t as a scalar.
ok, but its unrelated thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The bravest are surely those who have the clearest vision of what is before them, glory and danger alike, and yet notwithstanding go out to meet it. -- Thucydides
participants (4)
-
Andreas Rheinhardt -
James Almer -
Lynne -
Michael Niedermayer