* [FFmpeg-devel] [PATCH 1/3] avcodec/cbs_h266_syntax_template: Don't omit unused function parameter
@ 2024-02-21 23:31 Andreas Rheinhardt
2024-02-21 23:32 ` [FFmpeg-devel] [PATCH 2/3] avutil/opt: Use correct function pointer type Andreas Rheinhardt
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Andreas Rheinhardt @ 2024-02-21 23:31 UTC (permalink / raw)
To: ffmpeg-devel; +Cc: Andreas Rheinhardt
The calls to the sei_decoded_picture_hash read and write functions
are performed with four pointer arguments; just because one
of them is unused by the callees does not mean that they
can be omitted: This is undefined behaviour.
(This was not recognized because the SEI_MESSAGE_RW macro
contains casts.)
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
I found this via UBSan test failures with Clang 17,
but actually there are compiler warnings for this:
-Wcast-function-type for GCC and old Clang
and -Wcast-function-type in conjunction with
-Wno-cast-function-type-strict (without the latter,
Clang warns about conversions with different pointer
types, like int (*)(void*)->int (*)(struct Foo*);
the latter typically lead to UB (when used in a call),
but are actually common).
libavcodec/cbs_h266_syntax_template.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libavcodec/cbs_h266_syntax_template.c b/libavcodec/cbs_h266_syntax_template.c
index 26ee7a420b..e75f2f6971 100644
--- a/libavcodec/cbs_h266_syntax_template.c
+++ b/libavcodec/cbs_h266_syntax_template.c
@@ -3430,7 +3430,7 @@ static int FUNC(slice_header) (CodedBitstreamContext *ctx, RWContext *rw,
static int FUNC(sei_decoded_picture_hash) (CodedBitstreamContext *ctx,
RWContext *rw,
H266RawSEIDecodedPictureHash *
- current)
+ current, SEIMessageState *unused)
{
int err, c_idx, i;
--
2.40.1
_______________________________________________
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".
^ permalink raw reply [flat|nested] 7+ messages in thread* [FFmpeg-devel] [PATCH 2/3] avutil/opt: Use correct function pointer type 2024-02-21 23:31 [FFmpeg-devel] [PATCH 1/3] avcodec/cbs_h266_syntax_template: Don't omit unused function parameter Andreas Rheinhardt @ 2024-02-21 23:32 ` Andreas Rheinhardt 2024-02-24 12:09 ` Mark Thompson 2024-02-21 23:32 ` [FFmpeg-devel] [PATCH 3/3] avcodec/cbs_h2645: Avoid function pointer casts, fix UB Andreas Rheinhardt 2024-02-24 2:51 ` [FFmpeg-devel] [PATCH 1/3] avcodec/cbs_h266_syntax_template: Don't omit unused function parameter Andreas Rheinhardt 2 siblings, 1 reply; 7+ messages in thread From: Andreas Rheinhardt @ 2024-02-21 23:32 UTC (permalink / raw) To: ffmpeg-devel; +Cc: Andreas Rheinhardt av_get_sample/pix_fmt() return their respective enums and are therefore not of the type int (*)(const char*), yet they are called as-if they were of this type. This works in practice, but is actually undefined behaviour. With Clang 17 UBSan these violations are flagged, affecting lots of tests. The number of failing tests went down from 3363 to 164 here with this patch. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> --- libavutil/opt.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/libavutil/opt.c b/libavutil/opt.c index d13b1ab504..0681b19896 100644 --- a/libavutil/opt.c +++ b/libavutil/opt.c @@ -444,16 +444,26 @@ static int set_string_fmt(void *obj, const AVOption *o, const char *val, uint8_t return 0; } +static int get_pix_fmt(const char *name) +{ + return av_get_pix_fmt(name); +} + static int set_string_pixel_fmt(void *obj, const AVOption *o, const char *val, uint8_t *dst) { return set_string_fmt(obj, o, val, dst, - AV_PIX_FMT_NB, av_get_pix_fmt, "pixel format"); + AV_PIX_FMT_NB, get_pix_fmt, "pixel format"); +} + +static int get_sample_fmt(const char *name) +{ + return av_get_sample_fmt(name); } static int set_string_sample_fmt(void *obj, const AVOption *o, const char *val, uint8_t *dst) { return set_string_fmt(obj, o, val, dst, - AV_SAMPLE_FMT_NB, av_get_sample_fmt, "sample format"); + AV_SAMPLE_FMT_NB, get_sample_fmt, "sample format"); } static int set_string_dict(void *obj, const AVOption *o, const char *val, uint8_t **dst) -- 2.40.1 _______________________________________________ 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". ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [FFmpeg-devel] [PATCH 2/3] avutil/opt: Use correct function pointer type 2024-02-21 23:32 ` [FFmpeg-devel] [PATCH 2/3] avutil/opt: Use correct function pointer type Andreas Rheinhardt @ 2024-02-24 12:09 ` Mark Thompson 0 siblings, 0 replies; 7+ messages in thread From: Mark Thompson @ 2024-02-24 12:09 UTC (permalink / raw) To: ffmpeg-devel On 21/02/2024 23:32, Andreas Rheinhardt wrote: > av_get_sample/pix_fmt() return their respective enums > and are therefore not of the type int (*)(const char*), > yet they are called as-if they were of this type. > This works in practice, but is actually undefined behaviour. > > With Clang 17 UBSan these violations are flagged, affecting lots > of tests. The number of failing tests went down from 3363 to 164 > here with this patch. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > libavutil/opt.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/libavutil/opt.c b/libavutil/opt.c > index d13b1ab504..0681b19896 100644 > --- a/libavutil/opt.c > +++ b/libavutil/opt.c > @@ -444,16 +444,26 @@ static int set_string_fmt(void *obj, const AVOption *o, const char *val, uint8_t > return 0; > } > > +static int get_pix_fmt(const char *name) > +{ > + return av_get_pix_fmt(name); > +} > + > static int set_string_pixel_fmt(void *obj, const AVOption *o, const char *val, uint8_t *dst) > { > return set_string_fmt(obj, o, val, dst, > - AV_PIX_FMT_NB, av_get_pix_fmt, "pixel format"); > + AV_PIX_FMT_NB, get_pix_fmt, "pixel format"); > +} > + > +static int get_sample_fmt(const char *name) > +{ > + return av_get_sample_fmt(name); > } > > static int set_string_sample_fmt(void *obj, const AVOption *o, const char *val, uint8_t *dst) > { > return set_string_fmt(obj, o, val, dst, > - AV_SAMPLE_FMT_NB, av_get_sample_fmt, "sample format"); > + AV_SAMPLE_FMT_NB, get_sample_fmt, "sample format"); > } > > static int set_string_dict(void *obj, const AVOption *o, const char *val, uint8_t **dst) 1 and 2 of this set LGTM. Thanks, - Mark _______________________________________________ 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". ^ permalink raw reply [flat|nested] 7+ messages in thread
* [FFmpeg-devel] [PATCH 3/3] avcodec/cbs_h2645: Avoid function pointer casts, fix UB 2024-02-21 23:31 [FFmpeg-devel] [PATCH 1/3] avcodec/cbs_h266_syntax_template: Don't omit unused function parameter Andreas Rheinhardt 2024-02-21 23:32 ` [FFmpeg-devel] [PATCH 2/3] avutil/opt: Use correct function pointer type Andreas Rheinhardt @ 2024-02-21 23:32 ` Andreas Rheinhardt 2024-02-24 12:06 ` Mark Thompson 2024-02-24 2:51 ` [FFmpeg-devel] [PATCH 1/3] avcodec/cbs_h266_syntax_template: Don't omit unused function parameter Andreas Rheinhardt 2 siblings, 1 reply; 7+ messages in thread From: Andreas Rheinhardt @ 2024-02-21 23:32 UTC (permalink / raw) To: ffmpeg-devel; +Cc: Andreas Rheinhardt The SEI message read/write functions are called via function pointers where the SEI message-specific context is passed as void*. But the actual function definitions use a pointer to their proper context in place of void*, making the calls undefined behaviour. Clang UBSan 17 warns about this. This commit fixes this by making the functions match the type of the call. This reduced the number of failing FATE tests with UBSan from 164 to 85 here. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> --- libavcodec/cbs_h264_syntax_template.c | 24 ++++++++++----------- libavcodec/cbs_h265_syntax_template.c | 31 +++++++++++++++++---------- libavcodec/cbs_h266_syntax_template.c | 6 +++--- libavcodec/cbs_sei.h | 8 +++---- libavcodec/cbs_sei_syntax_template.c | 23 ++++++++++++-------- 5 files changed, 52 insertions(+), 40 deletions(-) diff --git a/libavcodec/cbs_h264_syntax_template.c b/libavcodec/cbs_h264_syntax_template.c index 0f8bba4a0d..282cd24292 100644 --- a/libavcodec/cbs_h264_syntax_template.c +++ b/libavcodec/cbs_h264_syntax_template.c @@ -511,9 +511,9 @@ static int FUNC(pps)(CodedBitstreamContext *ctx, RWContext *rw, } static int FUNC(sei_buffering_period)(CodedBitstreamContext *ctx, RWContext *rw, - H264RawSEIBufferingPeriod *current, - SEIMessageState *sei) + void *current_, SEIMessageState *sei) { + H264RawSEIBufferingPeriod *current = current_; CodedBitstreamH264Context *h264 = ctx->priv_data; const H264RawSPS *sps; int err, i, length; @@ -605,9 +605,9 @@ static int FUNC(sei_pic_timestamp)(CodedBitstreamContext *ctx, RWContext *rw, } static int FUNC(sei_pic_timing)(CodedBitstreamContext *ctx, RWContext *rw, - H264RawSEIPicTiming *current, - SEIMessageState *sei) + void *current_, SEIMessageState *sei) { + H264RawSEIPicTiming *current = current_; CodedBitstreamH264Context *h264 = ctx->priv_data; const H264RawSPS *sps; int err; @@ -677,9 +677,9 @@ static int FUNC(sei_pic_timing)(CodedBitstreamContext *ctx, RWContext *rw, } static int FUNC(sei_pan_scan_rect)(CodedBitstreamContext *ctx, RWContext *rw, - H264RawSEIPanScanRect *current, - SEIMessageState *sei) + void *current_, SEIMessageState *sei) { + H264RawSEIPanScanRect *current = current_; int err, i; HEADER("Pan-Scan Rectangle"); @@ -704,9 +704,9 @@ static int FUNC(sei_pan_scan_rect)(CodedBitstreamContext *ctx, RWContext *rw, } static int FUNC(sei_recovery_point)(CodedBitstreamContext *ctx, RWContext *rw, - H264RawSEIRecoveryPoint *current, - SEIMessageState *sei) + void *current_, SEIMessageState *sei) { + H264RawSEIRecoveryPoint *current = current_; int err; HEADER("Recovery Point"); @@ -720,9 +720,9 @@ static int FUNC(sei_recovery_point)(CodedBitstreamContext *ctx, RWContext *rw, } static int FUNC(film_grain_characteristics)(CodedBitstreamContext *ctx, RWContext *rw, - H264RawFilmGrainCharacteristics *current, - SEIMessageState *state) + void *current_, SEIMessageState *state) { + H264RawFilmGrainCharacteristics *current = current_; CodedBitstreamH264Context *h264 = ctx->priv_data; const H264RawSPS *sps; int err, c, i, j; @@ -803,9 +803,9 @@ static int FUNC(film_grain_characteristics)(CodedBitstreamContext *ctx, RWContex } static int FUNC(sei_display_orientation)(CodedBitstreamContext *ctx, RWContext *rw, - H264RawSEIDisplayOrientation *current, - SEIMessageState *sei) + void *current_, SEIMessageState *sei) { + H264RawSEIDisplayOrientation *current = current_; int err; HEADER("Display Orientation"); diff --git a/libavcodec/cbs_h265_syntax_template.c b/libavcodec/cbs_h265_syntax_template.c index 2d4b954718..53ae0cabff 100644 --- a/libavcodec/cbs_h265_syntax_template.c +++ b/libavcodec/cbs_h265_syntax_template.c @@ -1620,8 +1620,9 @@ static int FUNC(slice_segment_header)(CodedBitstreamContext *ctx, RWContext *rw, static int FUNC(sei_buffering_period) (CodedBitstreamContext *ctx, RWContext *rw, - H265RawSEIBufferingPeriod *current, SEIMessageState *sei) + void *current_, SEIMessageState *sei) { + H265RawSEIBufferingPeriod *current = current_; CodedBitstreamH265Context *h265 = ctx->priv_data; const H265RawSPS *sps; const H265RawHRDParameters *hrd; @@ -1730,8 +1731,9 @@ static int FUNC(sei_buffering_period) static int FUNC(sei_pic_timing) (CodedBitstreamContext *ctx, RWContext *rw, - H265RawSEIPicTiming *current, SEIMessageState *sei) + void *current_, SEIMessageState *sei) { + H265RawSEIPicTiming *current = current_; CodedBitstreamH265Context *h265 = ctx->priv_data; const H265RawSPS *sps; const H265RawHRDParameters *hrd; @@ -1806,8 +1808,9 @@ static int FUNC(sei_pic_timing) static int FUNC(sei_pan_scan_rect) (CodedBitstreamContext *ctx, RWContext *rw, - H265RawSEIPanScanRect *current, SEIMessageState *sei) + void *current_, SEIMessageState *sei) { + H265RawSEIPanScanRect *current = current_; int err, i; HEADER("Pan-Scan Rectangle"); @@ -1833,8 +1836,9 @@ static int FUNC(sei_pan_scan_rect) static int FUNC(sei_recovery_point) (CodedBitstreamContext *ctx, RWContext *rw, - H265RawSEIRecoveryPoint *current, SEIMessageState *sei) + void *current_, SEIMessageState *sei) { + H265RawSEIRecoveryPoint *current = current_; int err; HEADER("Recovery Point"); @@ -1848,9 +1852,9 @@ static int FUNC(sei_recovery_point) } static int FUNC(film_grain_characteristics)(CodedBitstreamContext *ctx, RWContext *rw, - H265RawFilmGrainCharacteristics *current, - SEIMessageState *state) + void *current_, SEIMessageState *state) { + H265RawFilmGrainCharacteristics *current = current_; CodedBitstreamH265Context *h265 = ctx->priv_data; const H265RawSPS *sps = h265->active_sps; int err, c, i, j; @@ -1914,8 +1918,9 @@ static int FUNC(film_grain_characteristics)(CodedBitstreamContext *ctx, RWContex static int FUNC(sei_display_orientation) (CodedBitstreamContext *ctx, RWContext *rw, - H265RawSEIDisplayOrientation *current, SEIMessageState *sei) + void *current_, SEIMessageState *sei) { + H265RawSEIDisplayOrientation *current = current_; int err; HEADER("Display Orientation"); @@ -1933,8 +1938,9 @@ static int FUNC(sei_display_orientation) static int FUNC(sei_active_parameter_sets) (CodedBitstreamContext *ctx, RWContext *rw, - H265RawSEIActiveParameterSets *current, SEIMessageState *sei) + void *current_, SEIMessageState *sei) { + H265RawSEIActiveParameterSets *current = current_; CodedBitstreamH265Context *h265 = ctx->priv_data; const H265RawVPS *vps; int err, i; @@ -1970,8 +1976,9 @@ static int FUNC(sei_active_parameter_sets) static int FUNC(sei_decoded_picture_hash) (CodedBitstreamContext *ctx, RWContext *rw, - H265RawSEIDecodedPictureHash *current, SEIMessageState *sei) + void *current_, SEIMessageState *sei) { + H265RawSEIDecodedPictureHash *current = current_; CodedBitstreamH265Context *h265 = ctx->priv_data; const H265RawSPS *sps = h265->active_sps; int err, c, i; @@ -2002,8 +2009,9 @@ static int FUNC(sei_decoded_picture_hash) static int FUNC(sei_time_code) (CodedBitstreamContext *ctx, RWContext *rw, - H265RawSEITimeCode *current, SEIMessageState *sei) + void *current_, SEIMessageState *sei) { + H265RawSEITimeCode *current = current_; int err, i; HEADER("Time Code"); @@ -2053,8 +2061,9 @@ static int FUNC(sei_time_code) static int FUNC(sei_alpha_channel_info) (CodedBitstreamContext *ctx, RWContext *rw, - H265RawSEIAlphaChannelInfo *current, SEIMessageState *sei) + void *current_, SEIMessageState *sei) { + H265RawSEIAlphaChannelInfo *current = current_; int err, length; HEADER("Alpha Channel Information"); diff --git a/libavcodec/cbs_h266_syntax_template.c b/libavcodec/cbs_h266_syntax_template.c index e75f2f6971..8a2adeb9e4 100644 --- a/libavcodec/cbs_h266_syntax_template.c +++ b/libavcodec/cbs_h266_syntax_template.c @@ -3428,10 +3428,10 @@ static int FUNC(slice_header) (CodedBitstreamContext *ctx, RWContext *rw, } static int FUNC(sei_decoded_picture_hash) (CodedBitstreamContext *ctx, - RWContext *rw, - H266RawSEIDecodedPictureHash * - current, SEIMessageState *unused) + RWContext *rw, void *current_, + SEIMessageState *unused) { + H266RawSEIDecodedPictureHash *current = current_; int err, c_idx, i; HEADER("Decoded Picture Hash"); diff --git a/libavcodec/cbs_sei.h b/libavcodec/cbs_sei.h index 4511c506cc..6d1bed4171 100644 --- a/libavcodec/cbs_sei.h +++ b/libavcodec/cbs_sei.h @@ -126,12 +126,10 @@ typedef struct SEIMessageTypeDescriptor { SEIMessageWriteFunction write; } SEIMessageTypeDescriptor; -// Macro for the read/write pair. The clumsy cast is needed because the -// current pointer is typed in all of the read/write functions but has to -// be void here to fit all cases. +// Macro for the read/write pair. #define SEI_MESSAGE_RW(codec, name) \ - .read = (SEIMessageReadFunction) cbs_ ## codec ## _read_ ## name, \ - .write = (SEIMessageWriteFunction)cbs_ ## codec ## _write_ ## name + .read = cbs_ ## codec ## _read_ ## name, \ + .write = cbs_ ## codec ## _write_ ## name // End-of-list sentinel element. #define SEI_MESSAGE_TYPE_END { .type = -1 } diff --git a/libavcodec/cbs_sei_syntax_template.c b/libavcodec/cbs_sei_syntax_template.c index 62dd1dabaa..16d2cbc406 100644 --- a/libavcodec/cbs_sei_syntax_template.c +++ b/libavcodec/cbs_sei_syntax_template.c @@ -18,8 +18,9 @@ static int FUNC(filler_payload) (CodedBitstreamContext *ctx, RWContext *rw, - SEIRawFillerPayload *current, SEIMessageState *state) + void *current_, SEIMessageState *state) { + SEIRawFillerPayload *current = current_; int err, i; HEADER("Filler Payload"); @@ -36,8 +37,9 @@ static int FUNC(filler_payload) static int FUNC(user_data_registered) (CodedBitstreamContext *ctx, RWContext *rw, - SEIRawUserDataRegistered *current, SEIMessageState *state) + void *current_, SEIMessageState *state) { + SEIRawUserDataRegistered *current = current_; int err, i, j; HEADER("User Data Registered ITU-T T.35"); @@ -68,8 +70,9 @@ static int FUNC(user_data_registered) static int FUNC(user_data_unregistered) (CodedBitstreamContext *ctx, RWContext *rw, - SEIRawUserDataUnregistered *current, SEIMessageState *state) + void *current_, SEIMessageState *state) { + SEIRawUserDataUnregistered *current = current_; int err, i; HEADER("User Data Unregistered"); @@ -96,8 +99,9 @@ static int FUNC(user_data_unregistered) static int FUNC(mastering_display_colour_volume) (CodedBitstreamContext *ctx, RWContext *rw, - SEIRawMasteringDisplayColourVolume *current, SEIMessageState *state) + void *current_, SEIMessageState *state) { + SEIRawMasteringDisplayColourVolume *current = current_; int err, c; HEADER("Mastering Display Colour Volume"); @@ -118,8 +122,9 @@ static int FUNC(mastering_display_colour_volume) static int FUNC(content_light_level_info) (CodedBitstreamContext *ctx, RWContext *rw, - SEIRawContentLightLevelInfo *current, SEIMessageState *state) + void *current_, SEIMessageState *state) { + SEIRawContentLightLevelInfo *current = current_; int err; HEADER("Content Light Level Information"); @@ -132,9 +137,9 @@ static int FUNC(content_light_level_info) static int FUNC(alternative_transfer_characteristics) (CodedBitstreamContext *ctx, RWContext *rw, - SEIRawAlternativeTransferCharacteristics *current, - SEIMessageState *state) + void *current_, SEIMessageState *state) { + SEIRawAlternativeTransferCharacteristics *current = current_; int err; HEADER("Alternative Transfer Characteristics"); @@ -146,9 +151,9 @@ static int FUNC(alternative_transfer_characteristics) static int FUNC(ambient_viewing_environment) (CodedBitstreamContext *ctx, RWContext *rw, - SEIRawAmbientViewingEnvironment *current, - SEIMessageState *state) + void *current_, SEIMessageState *state) { + SEIRawAmbientViewingEnvironment *current = current_; static const uint16_t max_ambient_light_value = 50000; int err; -- 2.40.1 _______________________________________________ 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". ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [FFmpeg-devel] [PATCH 3/3] avcodec/cbs_h2645: Avoid function pointer casts, fix UB 2024-02-21 23:32 ` [FFmpeg-devel] [PATCH 3/3] avcodec/cbs_h2645: Avoid function pointer casts, fix UB Andreas Rheinhardt @ 2024-02-24 12:06 ` Mark Thompson 2024-02-24 14:46 ` Andreas Rheinhardt 0 siblings, 1 reply; 7+ messages in thread From: Mark Thompson @ 2024-02-24 12:06 UTC (permalink / raw) To: ffmpeg-devel On 21/02/2024 23:32, Andreas Rheinhardt wrote: > The SEI message read/write functions are called > via function pointers where the SEI message-specific > context is passed as void*. But the actual function > definitions use a pointer to their proper context > in place of void*, making the calls undefined behaviour. > Clang UBSan 17 warns about this. > > This commit fixes this by making the functions match > the type of the call. This reduced the number of failing > FATE tests with UBSan from 164 to 85 here. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > libavcodec/cbs_h264_syntax_template.c | 24 ++++++++++----------- > libavcodec/cbs_h265_syntax_template.c | 31 +++++++++++++++++---------- > libavcodec/cbs_h266_syntax_template.c | 6 +++--- > libavcodec/cbs_sei.h | 8 +++---- > libavcodec/cbs_sei_syntax_template.c | 23 ++++++++++++-------- > 5 files changed, 52 insertions(+), 40 deletions(-) > > diff --git a/libavcodec/cbs_h264_syntax_template.c b/libavcodec/cbs_h264_syntax_template.c > index 0f8bba4a0d..282cd24292 100644 > --- a/libavcodec/cbs_h264_syntax_template.c > +++ b/libavcodec/cbs_h264_syntax_template.c > @@ -511,9 +511,9 @@ static int FUNC(pps)(CodedBitstreamContext *ctx, RWContext *rw, > } > > static int FUNC(sei_buffering_period)(CodedBitstreamContext *ctx, RWContext *rw, > - H264RawSEIBufferingPeriod *current, > - SEIMessageState *sei) > + void *current_, SEIMessageState *sei) > { > + H264RawSEIBufferingPeriod *current = current_; > CodedBitstreamH264Context *h264 = ctx->priv_data; > const H264RawSPS *sps; > int err, i, length; > @@ -605,9 +605,9 @@ static int FUNC(sei_pic_timestamp)(CodedBitstreamContext *ctx, RWContext *rw, > } > > static int FUNC(sei_pic_timing)(CodedBitstreamContext *ctx, RWContext *rw, > - H264RawSEIPicTiming *current, > - SEIMessageState *sei) > + void *current_, SEIMessageState *sei) > { > + H264RawSEIPicTiming *current = current_; > CodedBitstreamH264Context *h264 = ctx->priv_data; > const H264RawSPS *sps; > int err; > @@ -677,9 +677,9 @@ static int FUNC(sei_pic_timing)(CodedBitstreamContext *ctx, RWContext *rw, > } > > static int FUNC(sei_pan_scan_rect)(CodedBitstreamContext *ctx, RWContext *rw, > - H264RawSEIPanScanRect *current, > - SEIMessageState *sei) > + void *current_, SEIMessageState *sei) > { > + H264RawSEIPanScanRect *current = current_; > int err, i; > > HEADER("Pan-Scan Rectangle"); > @@ -704,9 +704,9 @@ static int FUNC(sei_pan_scan_rect)(CodedBitstreamContext *ctx, RWContext *rw, > } > > static int FUNC(sei_recovery_point)(CodedBitstreamContext *ctx, RWContext *rw, > - H264RawSEIRecoveryPoint *current, > - SEIMessageState *sei) > + void *current_, SEIMessageState *sei) > { > + H264RawSEIRecoveryPoint *current = current_; > int err; > > HEADER("Recovery Point"); > @@ -720,9 +720,9 @@ static int FUNC(sei_recovery_point)(CodedBitstreamContext *ctx, RWContext *rw, > } > > static int FUNC(film_grain_characteristics)(CodedBitstreamContext *ctx, RWContext *rw, > - H264RawFilmGrainCharacteristics *current, > - SEIMessageState *state) > + void *current_, SEIMessageState *state) > { > + H264RawFilmGrainCharacteristics *current = current_; > CodedBitstreamH264Context *h264 = ctx->priv_data; > const H264RawSPS *sps; > int err, c, i, j; > @@ -803,9 +803,9 @@ static int FUNC(film_grain_characteristics)(CodedBitstreamContext *ctx, RWContex > } > > static int FUNC(sei_display_orientation)(CodedBitstreamContext *ctx, RWContext *rw, > - H264RawSEIDisplayOrientation *current, > - SEIMessageState *sei) > + void *current_, SEIMessageState *sei) > { > + H264RawSEIDisplayOrientation *current = current_; > int err; > > HEADER("Display Orientation"); > diff --git a/libavcodec/cbs_h265_syntax_template.c b/libavcodec/cbs_h265_syntax_template.c > index 2d4b954718..53ae0cabff 100644 > --- a/libavcodec/cbs_h265_syntax_template.c > +++ b/libavcodec/cbs_h265_syntax_template.c > @@ -1620,8 +1620,9 @@ static int FUNC(slice_segment_header)(CodedBitstreamContext *ctx, RWContext *rw, > > static int FUNC(sei_buffering_period) > (CodedBitstreamContext *ctx, RWContext *rw, > - H265RawSEIBufferingPeriod *current, SEIMessageState *sei) > + void *current_, SEIMessageState *sei) > { > + H265RawSEIBufferingPeriod *current = current_; > CodedBitstreamH265Context *h265 = ctx->priv_data; > const H265RawSPS *sps; > const H265RawHRDParameters *hrd; > @@ -1730,8 +1731,9 @@ static int FUNC(sei_buffering_period) > > static int FUNC(sei_pic_timing) > (CodedBitstreamContext *ctx, RWContext *rw, > - H265RawSEIPicTiming *current, SEIMessageState *sei) > + void *current_, SEIMessageState *sei) > { > + H265RawSEIPicTiming *current = current_; > CodedBitstreamH265Context *h265 = ctx->priv_data; > const H265RawSPS *sps; > const H265RawHRDParameters *hrd; > @@ -1806,8 +1808,9 @@ static int FUNC(sei_pic_timing) > > static int FUNC(sei_pan_scan_rect) > (CodedBitstreamContext *ctx, RWContext *rw, > - H265RawSEIPanScanRect *current, SEIMessageState *sei) > + void *current_, SEIMessageState *sei) > { > + H265RawSEIPanScanRect *current = current_; > int err, i; > > HEADER("Pan-Scan Rectangle"); > @@ -1833,8 +1836,9 @@ static int FUNC(sei_pan_scan_rect) > > static int FUNC(sei_recovery_point) > (CodedBitstreamContext *ctx, RWContext *rw, > - H265RawSEIRecoveryPoint *current, SEIMessageState *sei) > + void *current_, SEIMessageState *sei) > { > + H265RawSEIRecoveryPoint *current = current_; > int err; > > HEADER("Recovery Point"); > @@ -1848,9 +1852,9 @@ static int FUNC(sei_recovery_point) > } > > static int FUNC(film_grain_characteristics)(CodedBitstreamContext *ctx, RWContext *rw, > - H265RawFilmGrainCharacteristics *current, > - SEIMessageState *state) > + void *current_, SEIMessageState *state) > { > + H265RawFilmGrainCharacteristics *current = current_; > CodedBitstreamH265Context *h265 = ctx->priv_data; > const H265RawSPS *sps = h265->active_sps; > int err, c, i, j; > @@ -1914,8 +1918,9 @@ static int FUNC(film_grain_characteristics)(CodedBitstreamContext *ctx, RWContex > > static int FUNC(sei_display_orientation) > (CodedBitstreamContext *ctx, RWContext *rw, > - H265RawSEIDisplayOrientation *current, SEIMessageState *sei) > + void *current_, SEIMessageState *sei) > { > + H265RawSEIDisplayOrientation *current = current_; > int err; > > HEADER("Display Orientation"); > @@ -1933,8 +1938,9 @@ static int FUNC(sei_display_orientation) > > static int FUNC(sei_active_parameter_sets) > (CodedBitstreamContext *ctx, RWContext *rw, > - H265RawSEIActiveParameterSets *current, SEIMessageState *sei) > + void *current_, SEIMessageState *sei) > { > + H265RawSEIActiveParameterSets *current = current_; > CodedBitstreamH265Context *h265 = ctx->priv_data; > const H265RawVPS *vps; > int err, i; > @@ -1970,8 +1976,9 @@ static int FUNC(sei_active_parameter_sets) > > static int FUNC(sei_decoded_picture_hash) > (CodedBitstreamContext *ctx, RWContext *rw, > - H265RawSEIDecodedPictureHash *current, SEIMessageState *sei) > + void *current_, SEIMessageState *sei) > { > + H265RawSEIDecodedPictureHash *current = current_; > CodedBitstreamH265Context *h265 = ctx->priv_data; > const H265RawSPS *sps = h265->active_sps; > int err, c, i; > @@ -2002,8 +2009,9 @@ static int FUNC(sei_decoded_picture_hash) > > static int FUNC(sei_time_code) > (CodedBitstreamContext *ctx, RWContext *rw, > - H265RawSEITimeCode *current, SEIMessageState *sei) > + void *current_, SEIMessageState *sei) > { > + H265RawSEITimeCode *current = current_; > int err, i; > > HEADER("Time Code"); > @@ -2053,8 +2061,9 @@ static int FUNC(sei_time_code) > > static int FUNC(sei_alpha_channel_info) > (CodedBitstreamContext *ctx, RWContext *rw, > - H265RawSEIAlphaChannelInfo *current, SEIMessageState *sei) > + void *current_, SEIMessageState *sei) > { > + H265RawSEIAlphaChannelInfo *current = current_; > int err, length; > > HEADER("Alpha Channel Information"); > diff --git a/libavcodec/cbs_h266_syntax_template.c b/libavcodec/cbs_h266_syntax_template.c > index e75f2f6971..8a2adeb9e4 100644 > --- a/libavcodec/cbs_h266_syntax_template.c > +++ b/libavcodec/cbs_h266_syntax_template.c > @@ -3428,10 +3428,10 @@ static int FUNC(slice_header) (CodedBitstreamContext *ctx, RWContext *rw, > } > > static int FUNC(sei_decoded_picture_hash) (CodedBitstreamContext *ctx, > - RWContext *rw, > - H266RawSEIDecodedPictureHash * > - current, SEIMessageState *unused) > + RWContext *rw, void *current_, > + SEIMessageState *unused) > { > + H266RawSEIDecodedPictureHash *current = current_; > int err, c_idx, i; > > HEADER("Decoded Picture Hash"); > diff --git a/libavcodec/cbs_sei.h b/libavcodec/cbs_sei.h > index 4511c506cc..6d1bed4171 100644 > --- a/libavcodec/cbs_sei.h > +++ b/libavcodec/cbs_sei.h > @@ -126,12 +126,10 @@ typedef struct SEIMessageTypeDescriptor { > SEIMessageWriteFunction write; > } SEIMessageTypeDescriptor; > > -// Macro for the read/write pair. The clumsy cast is needed because the > -// current pointer is typed in all of the read/write functions but has to > -// be void here to fit all cases. > +// Macro for the read/write pair. > #define SEI_MESSAGE_RW(codec, name) \ > - .read = (SEIMessageReadFunction) cbs_ ## codec ## _read_ ## name, \ > - .write = (SEIMessageWriteFunction)cbs_ ## codec ## _write_ ## name > + .read = cbs_ ## codec ## _read_ ## name, \ > + .write = cbs_ ## codec ## _write_ ## name > > // End-of-list sentinel element. > #define SEI_MESSAGE_TYPE_END { .type = -1 } > diff --git a/libavcodec/cbs_sei_syntax_template.c b/libavcodec/cbs_sei_syntax_template.c > index 62dd1dabaa..16d2cbc406 100644 > --- a/libavcodec/cbs_sei_syntax_template.c > +++ b/libavcodec/cbs_sei_syntax_template.c > @@ -18,8 +18,9 @@ > > static int FUNC(filler_payload) > (CodedBitstreamContext *ctx, RWContext *rw, > - SEIRawFillerPayload *current, SEIMessageState *state) > + void *current_, SEIMessageState *state) > { > + SEIRawFillerPayload *current = current_; > int err, i; > > HEADER("Filler Payload"); > @@ -36,8 +37,9 @@ static int FUNC(filler_payload) > > static int FUNC(user_data_registered) > (CodedBitstreamContext *ctx, RWContext *rw, > - SEIRawUserDataRegistered *current, SEIMessageState *state) > + void *current_, SEIMessageState *state) > { > + SEIRawUserDataRegistered *current = current_; > int err, i, j; > > HEADER("User Data Registered ITU-T T.35"); > @@ -68,8 +70,9 @@ static int FUNC(user_data_registered) > > static int FUNC(user_data_unregistered) > (CodedBitstreamContext *ctx, RWContext *rw, > - SEIRawUserDataUnregistered *current, SEIMessageState *state) > + void *current_, SEIMessageState *state) > { > + SEIRawUserDataUnregistered *current = current_; > int err, i; > > HEADER("User Data Unregistered"); > @@ -96,8 +99,9 @@ static int FUNC(user_data_unregistered) > > static int FUNC(mastering_display_colour_volume) > (CodedBitstreamContext *ctx, RWContext *rw, > - SEIRawMasteringDisplayColourVolume *current, SEIMessageState *state) > + void *current_, SEIMessageState *state) > { > + SEIRawMasteringDisplayColourVolume *current = current_; > int err, c; > > HEADER("Mastering Display Colour Volume"); > @@ -118,8 +122,9 @@ static int FUNC(mastering_display_colour_volume) > > static int FUNC(content_light_level_info) > (CodedBitstreamContext *ctx, RWContext *rw, > - SEIRawContentLightLevelInfo *current, SEIMessageState *state) > + void *current_, SEIMessageState *state) > { > + SEIRawContentLightLevelInfo *current = current_; > int err; > > HEADER("Content Light Level Information"); > @@ -132,9 +137,9 @@ static int FUNC(content_light_level_info) > > static int FUNC(alternative_transfer_characteristics) > (CodedBitstreamContext *ctx, RWContext *rw, > - SEIRawAlternativeTransferCharacteristics *current, > - SEIMessageState *state) > + void *current_, SEIMessageState *state) > { > + SEIRawAlternativeTransferCharacteristics *current = current_; > int err; > > HEADER("Alternative Transfer Characteristics"); > @@ -146,9 +151,9 @@ static int FUNC(alternative_transfer_characteristics) > > static int FUNC(ambient_viewing_environment) > (CodedBitstreamContext *ctx, RWContext *rw, > - SEIRawAmbientViewingEnvironment *current, > - SEIMessageState *state) > + void *current_, SEIMessageState *state) > { > + SEIRawAmbientViewingEnvironment *current = current_; > static const uint16_t max_ambient_light_value = 50000; > int err; > I agree that we should fix the UB, but pushing the ugliness into the clean parts of the template code does not seem like the right way to do it. Since there is exactly one callsite (in code, compiled to two), can we restrict the nastiness of it to only appear there? (It would be straightforward to have all possible callees visible at that moment by moving the SEI template include after the per-codec ones. Then make the function pointers void* and have a switch over them to call it with the right type, perhaps?) Thanks, - Mark _______________________________________________ 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". ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [FFmpeg-devel] [PATCH 3/3] avcodec/cbs_h2645: Avoid function pointer casts, fix UB 2024-02-24 12:06 ` Mark Thompson @ 2024-02-24 14:46 ` Andreas Rheinhardt 0 siblings, 0 replies; 7+ messages in thread From: Andreas Rheinhardt @ 2024-02-24 14:46 UTC (permalink / raw) To: ffmpeg-devel Mark Thompson: > On 21/02/2024 23:32, Andreas Rheinhardt wrote: >> The SEI message read/write functions are called >> via function pointers where the SEI message-specific >> context is passed as void*. But the actual function >> definitions use a pointer to their proper context >> in place of void*, making the calls undefined behaviour. >> Clang UBSan 17 warns about this. >> >> This commit fixes this by making the functions match >> the type of the call. This reduced the number of failing >> FATE tests with UBSan from 164 to 85 here. >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> >> --- >> libavcodec/cbs_h264_syntax_template.c | 24 ++++++++++----------- >> libavcodec/cbs_h265_syntax_template.c | 31 +++++++++++++++++---------- >> libavcodec/cbs_h266_syntax_template.c | 6 +++--- >> libavcodec/cbs_sei.h | 8 +++---- >> libavcodec/cbs_sei_syntax_template.c | 23 ++++++++++++-------- >> 5 files changed, 52 insertions(+), 40 deletions(-) >> >> diff --git a/libavcodec/cbs_h264_syntax_template.c >> b/libavcodec/cbs_h264_syntax_template.c >> index 0f8bba4a0d..282cd24292 100644 >> --- a/libavcodec/cbs_h264_syntax_template.c >> +++ b/libavcodec/cbs_h264_syntax_template.c >> @@ -511,9 +511,9 @@ static int FUNC(pps)(CodedBitstreamContext *ctx, >> RWContext *rw, >> } >> static int FUNC(sei_buffering_period)(CodedBitstreamContext *ctx, >> RWContext *rw, >> - H264RawSEIBufferingPeriod >> *current, >> - SEIMessageState *sei) >> + void *current_, SEIMessageState >> *sei) >> { >> + H264RawSEIBufferingPeriod *current = current_; >> CodedBitstreamH264Context *h264 = ctx->priv_data; >> const H264RawSPS *sps; >> int err, i, length; >> @@ -605,9 +605,9 @@ static int >> FUNC(sei_pic_timestamp)(CodedBitstreamContext *ctx, RWContext *rw, >> } >> static int FUNC(sei_pic_timing)(CodedBitstreamContext *ctx, >> RWContext *rw, >> - H264RawSEIPicTiming *current, >> - SEIMessageState *sei) >> + void *current_, SEIMessageState *sei) >> { >> + H264RawSEIPicTiming *current = current_; >> CodedBitstreamH264Context *h264 = ctx->priv_data; >> const H264RawSPS *sps; >> int err; >> @@ -677,9 +677,9 @@ static int >> FUNC(sei_pic_timing)(CodedBitstreamContext *ctx, RWContext *rw, >> } >> static int FUNC(sei_pan_scan_rect)(CodedBitstreamContext *ctx, >> RWContext *rw, >> - H264RawSEIPanScanRect *current, >> - SEIMessageState *sei) >> + void *current_, SEIMessageState *sei) >> { >> + H264RawSEIPanScanRect *current = current_; >> int err, i; >> HEADER("Pan-Scan Rectangle"); >> @@ -704,9 +704,9 @@ static int >> FUNC(sei_pan_scan_rect)(CodedBitstreamContext *ctx, RWContext *rw, >> } >> static int FUNC(sei_recovery_point)(CodedBitstreamContext *ctx, >> RWContext *rw, >> - H264RawSEIRecoveryPoint *current, >> - SEIMessageState *sei) >> + void *current_, SEIMessageState >> *sei) >> { >> + H264RawSEIRecoveryPoint *current = current_; >> int err; >> HEADER("Recovery Point"); >> @@ -720,9 +720,9 @@ static int >> FUNC(sei_recovery_point)(CodedBitstreamContext *ctx, RWContext *rw, >> } >> static int FUNC(film_grain_characteristics)(CodedBitstreamContext >> *ctx, RWContext *rw, >> - >> H264RawFilmGrainCharacteristics *current, >> - SEIMessageState *state) >> + void *current_, >> SEIMessageState *state) >> { >> + H264RawFilmGrainCharacteristics *current = current_; >> CodedBitstreamH264Context *h264 = ctx->priv_data; >> const H264RawSPS *sps; >> int err, c, i, j; >> @@ -803,9 +803,9 @@ static int >> FUNC(film_grain_characteristics)(CodedBitstreamContext *ctx, RWContex >> } >> static int FUNC(sei_display_orientation)(CodedBitstreamContext >> *ctx, RWContext *rw, >> - H264RawSEIDisplayOrientation >> *current, >> - SEIMessageState *sei) >> + void *current_, >> SEIMessageState *sei) >> { >> + H264RawSEIDisplayOrientation *current = current_; >> int err; >> HEADER("Display Orientation"); >> diff --git a/libavcodec/cbs_h265_syntax_template.c >> b/libavcodec/cbs_h265_syntax_template.c >> index 2d4b954718..53ae0cabff 100644 >> --- a/libavcodec/cbs_h265_syntax_template.c >> +++ b/libavcodec/cbs_h265_syntax_template.c >> @@ -1620,8 +1620,9 @@ static int >> FUNC(slice_segment_header)(CodedBitstreamContext *ctx, RWContext *rw, >> static int FUNC(sei_buffering_period) >> (CodedBitstreamContext *ctx, RWContext *rw, >> - H265RawSEIBufferingPeriod *current, SEIMessageState *sei) >> + void *current_, SEIMessageState *sei) >> { >> + H265RawSEIBufferingPeriod *current = current_; >> CodedBitstreamH265Context *h265 = ctx->priv_data; >> const H265RawSPS *sps; >> const H265RawHRDParameters *hrd; >> @@ -1730,8 +1731,9 @@ static int FUNC(sei_buffering_period) >> static int FUNC(sei_pic_timing) >> (CodedBitstreamContext *ctx, RWContext *rw, >> - H265RawSEIPicTiming *current, SEIMessageState *sei) >> + void *current_, SEIMessageState *sei) >> { >> + H265RawSEIPicTiming *current = current_; >> CodedBitstreamH265Context *h265 = ctx->priv_data; >> const H265RawSPS *sps; >> const H265RawHRDParameters *hrd; >> @@ -1806,8 +1808,9 @@ static int FUNC(sei_pic_timing) >> static int FUNC(sei_pan_scan_rect) >> (CodedBitstreamContext *ctx, RWContext *rw, >> - H265RawSEIPanScanRect *current, SEIMessageState *sei) >> + void *current_, SEIMessageState *sei) >> { >> + H265RawSEIPanScanRect *current = current_; >> int err, i; >> HEADER("Pan-Scan Rectangle"); >> @@ -1833,8 +1836,9 @@ static int FUNC(sei_pan_scan_rect) >> static int FUNC(sei_recovery_point) >> (CodedBitstreamContext *ctx, RWContext *rw, >> - H265RawSEIRecoveryPoint *current, SEIMessageState *sei) >> + void *current_, SEIMessageState *sei) >> { >> + H265RawSEIRecoveryPoint *current = current_; >> int err; >> HEADER("Recovery Point"); >> @@ -1848,9 +1852,9 @@ static int FUNC(sei_recovery_point) >> } >> static int FUNC(film_grain_characteristics)(CodedBitstreamContext >> *ctx, RWContext *rw, >> - >> H265RawFilmGrainCharacteristics *current, >> - SEIMessageState *state) >> + void *current_, >> SEIMessageState *state) >> { >> + H265RawFilmGrainCharacteristics *current = current_; >> CodedBitstreamH265Context *h265 = ctx->priv_data; >> const H265RawSPS *sps = h265->active_sps; >> int err, c, i, j; >> @@ -1914,8 +1918,9 @@ static int >> FUNC(film_grain_characteristics)(CodedBitstreamContext *ctx, RWContex >> static int FUNC(sei_display_orientation) >> (CodedBitstreamContext *ctx, RWContext *rw, >> - H265RawSEIDisplayOrientation *current, SEIMessageState *sei) >> + void *current_, SEIMessageState *sei) >> { >> + H265RawSEIDisplayOrientation *current = current_; >> int err; >> HEADER("Display Orientation"); >> @@ -1933,8 +1938,9 @@ static int FUNC(sei_display_orientation) >> static int FUNC(sei_active_parameter_sets) >> (CodedBitstreamContext *ctx, RWContext *rw, >> - H265RawSEIActiveParameterSets *current, SEIMessageState *sei) >> + void *current_, SEIMessageState *sei) >> { >> + H265RawSEIActiveParameterSets *current = current_; >> CodedBitstreamH265Context *h265 = ctx->priv_data; >> const H265RawVPS *vps; >> int err, i; >> @@ -1970,8 +1976,9 @@ static int FUNC(sei_active_parameter_sets) >> static int FUNC(sei_decoded_picture_hash) >> (CodedBitstreamContext *ctx, RWContext *rw, >> - H265RawSEIDecodedPictureHash *current, SEIMessageState *sei) >> + void *current_, SEIMessageState *sei) >> { >> + H265RawSEIDecodedPictureHash *current = current_; >> CodedBitstreamH265Context *h265 = ctx->priv_data; >> const H265RawSPS *sps = h265->active_sps; >> int err, c, i; >> @@ -2002,8 +2009,9 @@ static int FUNC(sei_decoded_picture_hash) >> static int FUNC(sei_time_code) >> (CodedBitstreamContext *ctx, RWContext *rw, >> - H265RawSEITimeCode *current, SEIMessageState *sei) >> + void *current_, SEIMessageState *sei) >> { >> + H265RawSEITimeCode *current = current_; >> int err, i; >> HEADER("Time Code"); >> @@ -2053,8 +2061,9 @@ static int FUNC(sei_time_code) >> static int FUNC(sei_alpha_channel_info) >> (CodedBitstreamContext *ctx, RWContext *rw, >> - H265RawSEIAlphaChannelInfo *current, SEIMessageState *sei) >> + void *current_, SEIMessageState *sei) >> { >> + H265RawSEIAlphaChannelInfo *current = current_; >> int err, length; >> HEADER("Alpha Channel Information"); >> diff --git a/libavcodec/cbs_h266_syntax_template.c >> b/libavcodec/cbs_h266_syntax_template.c >> index e75f2f6971..8a2adeb9e4 100644 >> --- a/libavcodec/cbs_h266_syntax_template.c >> +++ b/libavcodec/cbs_h266_syntax_template.c >> @@ -3428,10 +3428,10 @@ static int FUNC(slice_header) >> (CodedBitstreamContext *ctx, RWContext *rw, >> } >> static int FUNC(sei_decoded_picture_hash) (CodedBitstreamContext >> *ctx, >> - RWContext *rw, >> - >> H266RawSEIDecodedPictureHash * >> - current, SEIMessageState >> *unused) >> + RWContext *rw, void >> *current_, >> + SEIMessageState *unused) >> { >> + H266RawSEIDecodedPictureHash *current = current_; >> int err, c_idx, i; >> HEADER("Decoded Picture Hash"); >> diff --git a/libavcodec/cbs_sei.h b/libavcodec/cbs_sei.h >> index 4511c506cc..6d1bed4171 100644 >> --- a/libavcodec/cbs_sei.h >> +++ b/libavcodec/cbs_sei.h >> @@ -126,12 +126,10 @@ typedef struct SEIMessageTypeDescriptor { >> SEIMessageWriteFunction write; >> } SEIMessageTypeDescriptor; >> -// Macro for the read/write pair. The clumsy cast is needed >> because the >> -// current pointer is typed in all of the read/write functions but >> has to >> -// be void here to fit all cases. >> +// Macro for the read/write pair. >> #define SEI_MESSAGE_RW(codec, name) \ >> - .read = (SEIMessageReadFunction) cbs_ ## codec ## _read_ ## >> name, \ >> - .write = (SEIMessageWriteFunction)cbs_ ## codec ## _write_ ## name >> + .read = cbs_ ## codec ## _read_ ## name, \ >> + .write = cbs_ ## codec ## _write_ ## name >> // End-of-list sentinel element. >> #define SEI_MESSAGE_TYPE_END { .type = -1 } >> diff --git a/libavcodec/cbs_sei_syntax_template.c >> b/libavcodec/cbs_sei_syntax_template.c >> index 62dd1dabaa..16d2cbc406 100644 >> --- a/libavcodec/cbs_sei_syntax_template.c >> +++ b/libavcodec/cbs_sei_syntax_template.c >> @@ -18,8 +18,9 @@ >> static int FUNC(filler_payload) >> (CodedBitstreamContext *ctx, RWContext *rw, >> - SEIRawFillerPayload *current, SEIMessageState *state) >> + void *current_, SEIMessageState *state) >> { >> + SEIRawFillerPayload *current = current_; >> int err, i; >> HEADER("Filler Payload"); >> @@ -36,8 +37,9 @@ static int FUNC(filler_payload) >> static int FUNC(user_data_registered) >> (CodedBitstreamContext *ctx, RWContext *rw, >> - SEIRawUserDataRegistered *current, SEIMessageState *state) >> + void *current_, SEIMessageState *state) >> { >> + SEIRawUserDataRegistered *current = current_; >> int err, i, j; >> HEADER("User Data Registered ITU-T T.35"); >> @@ -68,8 +70,9 @@ static int FUNC(user_data_registered) >> static int FUNC(user_data_unregistered) >> (CodedBitstreamContext *ctx, RWContext *rw, >> - SEIRawUserDataUnregistered *current, SEIMessageState *state) >> + void *current_, SEIMessageState *state) >> { >> + SEIRawUserDataUnregistered *current = current_; >> int err, i; >> HEADER("User Data Unregistered"); >> @@ -96,8 +99,9 @@ static int FUNC(user_data_unregistered) >> static int FUNC(mastering_display_colour_volume) >> (CodedBitstreamContext *ctx, RWContext *rw, >> - SEIRawMasteringDisplayColourVolume *current, SEIMessageState >> *state) >> + void *current_, SEIMessageState *state) >> { >> + SEIRawMasteringDisplayColourVolume *current = current_; >> int err, c; >> HEADER("Mastering Display Colour Volume"); >> @@ -118,8 +122,9 @@ static int FUNC(mastering_display_colour_volume) >> static int FUNC(content_light_level_info) >> (CodedBitstreamContext *ctx, RWContext *rw, >> - SEIRawContentLightLevelInfo *current, SEIMessageState *state) >> + void *current_, SEIMessageState *state) >> { >> + SEIRawContentLightLevelInfo *current = current_; >> int err; >> HEADER("Content Light Level Information"); >> @@ -132,9 +137,9 @@ static int FUNC(content_light_level_info) >> static int FUNC(alternative_transfer_characteristics) >> (CodedBitstreamContext *ctx, RWContext *rw, >> - SEIRawAlternativeTransferCharacteristics *current, >> - SEIMessageState *state) >> + void *current_, SEIMessageState *state) >> { >> + SEIRawAlternativeTransferCharacteristics *current = current_; >> int err; >> HEADER("Alternative Transfer Characteristics"); >> @@ -146,9 +151,9 @@ static int FUNC(alternative_transfer_characteristics) >> static int FUNC(ambient_viewing_environment) >> (CodedBitstreamContext *ctx, RWContext *rw, >> - SEIRawAmbientViewingEnvironment *current, >> - SEIMessageState *state) >> + void *current_, SEIMessageState *state) >> { >> + SEIRawAmbientViewingEnvironment *current = current_; >> static const uint16_t max_ambient_light_value = 50000; >> int err; >> > > I agree that we should fix the UB, but pushing the ugliness into the > clean parts of the template code does not seem like the right way to do it. > > Since there is exactly one callsite (in code, compiled to two), can we > restrict the nastiness of it to only appear there? > > (It would be straightforward to have all possible callees visible at > that moment by moving the SEI template include after the per-codec > ones. Then make the function pointers void* and have a switch over them > to call it with the right type, perhaps?) > 1. Conversions between function pointers and pointers to object types or pointers to void are UB in itself (but are supported by all platforms we care about). But this is not an important point (it can be avoided by using the current types of SEIMessageRead/WriteFunction as intermediate types). 2. I do not really see uglyness in this approach. What you are proposing is IMO very ugly; if we already have a gigantic switch, we could just as well undo using function pointers at all and call the functions similar to how it was before 8843607f495c95c1e67a3ce3d6f15dca6e252439. I can send a patch doing that if you want. Would also have the advantage of moving SEIMessageTypeDescriptor to .rodata. 3. I see two more approaches to fix this. Both involve using a special SEI_FUNC macro instead of the common FUNC macro. This macro would take both the function name as well as the function arguments as parameters. a) In the first approach SEI_FUNC(name, CodedBitstreamContext *ctx, RWContext *rw, Type *current, SEIMessageState *state) would expand to FUNC(name)(CodedBitstreamContext *ctx, RWContext *rw, void *cur, SEIMessageState *state) { Type *current = cur; (FUNC would of course also expand.) The uglyness of this approach is obvious: There would be no opening { after SEI_FUNC(). b) In the second approach it would expand to static int FUNC(name##_internal)(CodedBitstreamContext *ctx, RWContext *rw, Type *current, SEIMessageState *state); static int FUNC(name)(CodedBitstreamContext *ctx, RWContext *rw, void *current, SEIMessageState *state) { return FUNC(name##_internal)(ctx, rw, current, state); } static int FUNC(name##_internal)(CodedBitstreamContext *ctx, RWContext *rw, Type *current, SEIMessageState *state) One downside of approach b) is that the __func__ macro would not work as expected; but we don't use it anyway right now. One could of course implement these SEI_FUNC macros as SEI_FUNC(name, type) only; then the type would be duplicated in the actual parameter list. Please tell me which approach you prefer. - Andreas _______________________________________________ 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". ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [FFmpeg-devel] [PATCH 1/3] avcodec/cbs_h266_syntax_template: Don't omit unused function parameter 2024-02-21 23:31 [FFmpeg-devel] [PATCH 1/3] avcodec/cbs_h266_syntax_template: Don't omit unused function parameter Andreas Rheinhardt 2024-02-21 23:32 ` [FFmpeg-devel] [PATCH 2/3] avutil/opt: Use correct function pointer type Andreas Rheinhardt 2024-02-21 23:32 ` [FFmpeg-devel] [PATCH 3/3] avcodec/cbs_h2645: Avoid function pointer casts, fix UB Andreas Rheinhardt @ 2024-02-24 2:51 ` Andreas Rheinhardt 2 siblings, 0 replies; 7+ messages in thread From: Andreas Rheinhardt @ 2024-02-24 2:51 UTC (permalink / raw) To: ffmpeg-devel Andreas Rheinhardt: > The calls to the sei_decoded_picture_hash read and write functions > are performed with four pointer arguments; just because one > of them is unused by the callees does not mean that they > can be omitted: This is undefined behaviour. > (This was not recognized because the SEI_MESSAGE_RW macro > contains casts.) > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > I found this via UBSan test failures with Clang 17, > but actually there are compiler warnings for this: > -Wcast-function-type for GCC and old Clang > and -Wcast-function-type in conjunction with > -Wno-cast-function-type-strict (without the latter, > Clang warns about conversions with different pointer > types, like int (*)(void*)->int (*)(struct Foo*); > the latter typically lead to UB (when used in a call), > but are actually common). > > libavcodec/cbs_h266_syntax_template.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavcodec/cbs_h266_syntax_template.c b/libavcodec/cbs_h266_syntax_template.c > index 26ee7a420b..e75f2f6971 100644 > --- a/libavcodec/cbs_h266_syntax_template.c > +++ b/libavcodec/cbs_h266_syntax_template.c > @@ -3430,7 +3430,7 @@ static int FUNC(slice_header) (CodedBitstreamContext *ctx, RWContext *rw, > static int FUNC(sei_decoded_picture_hash) (CodedBitstreamContext *ctx, > RWContext *rw, > H266RawSEIDecodedPictureHash * > - current) > + current, SEIMessageState *unused) > { > int err, c_idx, i; > Will apply this patchset tomorrow unless there are objections. - Andreas _______________________________________________ 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". ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-02-24 15:17 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-02-21 23:31 [FFmpeg-devel] [PATCH 1/3] avcodec/cbs_h266_syntax_template: Don't omit unused function parameter Andreas Rheinhardt 2024-02-21 23:32 ` [FFmpeg-devel] [PATCH 2/3] avutil/opt: Use correct function pointer type Andreas Rheinhardt 2024-02-24 12:09 ` Mark Thompson 2024-02-21 23:32 ` [FFmpeg-devel] [PATCH 3/3] avcodec/cbs_h2645: Avoid function pointer casts, fix UB Andreas Rheinhardt 2024-02-24 12:06 ` Mark Thompson 2024-02-24 14:46 ` Andreas Rheinhardt 2024-02-24 2:51 ` [FFmpeg-devel] [PATCH 1/3] avcodec/cbs_h266_syntax_template: Don't omit unused function parameter Andreas Rheinhardt
Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel This inbox may be cloned and mirrored by anyone: git clone --mirror https://master.gitmailbox.com/ffmpegdev/0 ffmpegdev/git/0.git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V2 ffmpegdev ffmpegdev/ https://master.gitmailbox.com/ffmpegdev \ ffmpegdev@gitmailbox.com public-inbox-index ffmpegdev Example config snippet for mirrors. AGPL code for this site: git clone https://public-inbox.org/public-inbox.git