From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by master.gitmailbox.com (Postfix) with ESMTP id DEC5349983 for ; Sat, 24 Feb 2024 12:06:26 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 23BE868C654; Sat, 24 Feb 2024 14:06:23 +0200 (EET) Received: from mail-wm1-f44.google.com (mail-wm1-f44.google.com [209.85.128.44]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id F412868BF4B for ; Sat, 24 Feb 2024 14:06:15 +0200 (EET) Received: by mail-wm1-f44.google.com with SMTP id 5b1f17b1804b1-41241f64c6bso12797225e9.0 for ; Sat, 24 Feb 2024 04:06:15 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=jkqxz-net.20230601.gappssmtp.com; s=20230601; t=1708776375; x=1709381175; darn=ffmpeg.org; h=content-transfer-encoding:in-reply-to:from:references:to :content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=3+/06t2AQVWJDaY1eAGfv6FF66xHIgRWXoNOVLvtUwA=; b=g1Y5/ty62swMv5PV7ftGEFZ/+Jd+rR7hI3wfZAs0JJxTDuQwn+tuM7Xst9Z6qb+ukZ xsQNWiw13vkFNH1EmuAEGLoX+eMygvkAFq2CspPR8DdIAGWJASYOmRcNUoQ1Jz4J8TFW XCcSLEzmde/B/DLnSwWim5M8RPJGumDHimeLXjsFgE/n4awIkBTaIdhkMbk+/C7zwHRi mdXp9epCCQH9ne/3eYRGMrG32FF5LPdG/PdY0ErnheWaSDgrA4y1yU2AghuSYUmjvep3 e0yfzien/t+gVSxnnvCZd4HVuvqc97gvd5LYEyy+HiMimIfT0qGhjlui61iY1nwAPlSj u6rg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1708776375; x=1709381175; h=content-transfer-encoding:in-reply-to:from:references:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=3+/06t2AQVWJDaY1eAGfv6FF66xHIgRWXoNOVLvtUwA=; b=Fwft6+ChhVnr8+5jyq3p0f32DShYdNH0dO+i712QZwyGFwl1HwnjwtBckUVUq/8gaJ H73ZUXKqrpWnxKjA8slgFLKNqW6Hpk5F+sbZBlFq4fcY6UTT5zCUCa+fU/Wawkb+RvPq 4oUkyKZGXQeJhCzuvWMBOJbPJmk4Dy+Tyv/ROqpf8gkbTkozyjAskHkQTlxZuLrYWA6y W78MJS9z56sDv1ZaBjQZk2SgQHcgX8ko8jpNIyT3uIjJpz86Ddp96WuMEoDolhZPuyPP zocOt9YboaC9spJilNODDKOX/FujqtX3CoKtuXqQtvf+JBTiQYueLH/N35cHsQ4IFov/ o29A== X-Gm-Message-State: AOJu0Yx0/ZG2pyMNWq3JW1dba7/VkTiMSPp1nN3B4H5y8zBj7u2Mv7/D hCNka49+BoxfXz16gr8FE/mpvUgoiDp4LTH+s9soHjIrDHNdmVkShx1R0UYZPfLTJm4wYZasGxH R X-Google-Smtp-Source: AGHT+IGF1PakSW5rc68cCONQwlip8hbU6sJm+LXugjMO5ZZte+F0qdDuyj0yUw0yi7EJHrBlThGCQQ== X-Received: by 2002:a05:600c:4ba8:b0:412:64a5:a21 with SMTP id e40-20020a05600c4ba800b0041264a50a21mr1502735wmp.40.1708776374936; Sat, 24 Feb 2024 04:06:14 -0800 (PST) Received: from [192.168.0.15] (cpc92302-cmbg19-2-0-cust1183.5-4.cable.virginm.net. [82.1.212.160]) by smtp.gmail.com with ESMTPSA id p15-20020a05600c468f00b004128f41a13fsm2084330wmo.38.2024.02.24.04.06.14 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sat, 24 Feb 2024 04:06:14 -0800 (PST) Message-ID: Date: Sat, 24 Feb 2024 12:06:43 +0000 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: ffmpeg-devel@ffmpeg.org References: From: Mark Thompson In-Reply-To: Subject: Re: [FFmpeg-devel] [PATCH 3/3] avcodec/cbs_h2645: Avoid function pointer casts, fix UB X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: 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 > --- > 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".