[FFmpeg-devel] [PATCH 01/12] fftools/textformat/avtextformat: Simplify avtext_print_rational()

[Andreas Rheinhardt]

softworkz .:
> 
> 
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
>> Andreas Rheinhardt
>> Sent: Mittwoch, 16. April 2025 06:28
>> To: ffmpeg-devel at ffmpeg.org
>> Subject: Re: [FFmpeg-devel] [PATCH 01/12]
>> fftools/textformat/avtextformat: Simplify avtext_print_rational()
>>
>> softworkz .:
>>>
>>>
>>>> -----Original Message-----
>>>> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
>>>> Andreas Rheinhardt
>>>> Sent: Dienstag, 15. April 2025 10:36
>>>> To: ffmpeg-devel at ffmpeg.org
>>>> Subject: Re: [FFmpeg-devel] [PATCH 01/12]
>>>> fftools/textformat/avtextformat: Simplify avtext_print_rational()
>>>>
>>>> softworkz .:
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
>>>>>> Andreas Rheinhardt
>>>>>> Sent: Dienstag, 15. April 2025 03:00
>>>>>> To: FFmpeg development discussions and patches <ffmpeg-
>>>>>> devel at ffmpeg.org>
>>>>>> Subject: [FFmpeg-devel] [PATCH 01/12]
>>>> fftools/textformat/avtextformat:
>>>>>> Simplify avtext_print_rational()
>>>>>>
>>>>>> Patches attached.
>>>>>>
>>>>>> - Andreas
>>>>>
>>>>>
>>>>> Hi Andreas,
>>>>>
>>>>> thanks a lot for working through this. I'll go over it tomorrow.
>>>>>
>>>>> As to not waste your time, it's probably best when we get those
>>>>> changes applied in a timely manner so that I can rebase the new
>>>>> patchset on top of it.
>>>>>
>>>>> Since you're sending the patches as attachments:
>>>>> How do you want me to reply with code context? Whole files or
>>>>> just snippets? And quoted?
>>>>>
>>>>
>>>> Snippets is better. So is quoted.
>>>>
>>>> - Andreas
>>>>
>>>
>>> Hi Andreas,
>>>
>>> thanks again for the well-spotted improvements. Just two notes:
>>>
>>>
>>> 0007-fftools-textformat-Use-av_default_item_name.patch
>>>
>>> In the new patchset, those macros are removed from the individual
>>> files. There's now a single macro in tf_internal.h and I've applied
>>> this change there.
>>
>> So you use a move/deduplicate commit to change something? Not good.
> 
> No. The patchset has a deduplication commit. That's what I've submitted
> to the ML already. 
> Now I made another commit locally (on top of that) which makes this change.
> 
> 
>>> 0008-fftools-textformat-avtextformat-Fix-segfault-upon-al.patch
>>> 0009-fftools-textformat-avtextformat-Fix-segfault-upon-al.patch
>>>
>>> Can this happen?
>>
>> Of course it can. All allocations can fail. That's why we check them.
>> Have you been coding with the assumption that allocations never fail?
>> (You can use av_max_alloc(1); to simulate allocation failures.)
> 
> Allocations can fail, but statically initialized global const values?

The pointers to said static objects are only set after having allocated
the private context. So the issue can happen (not with current master)
"if an AVTextFormatContext's private context could not be allocated" (as
has been said in a commit message; similarly for AVTextWriterContext).

(In case you are wondering why I did not set the pointers to
writer/formatter before having allocated the private context: The uninit
functions are only supposed to be called with the writer/formatter's
private context already allocated (otherwise all of them would need to
check for its existence), so this would not reduce the amount of checks.)

- Andreas