[FFmpeg-devel] [PATCH 14/14] avutil/avstring: Avoid av_strdup(NULL)

[Andreas Rheinhardt]

James Almer:
> On 3/25/2024 1:55 PM, Andreas Rheinhardt wrote:
>> Vittorio Giovara:
>>> On Mon, Mar 25, 2024 at 12:38 PM Andreas Rheinhardt <
>>> andreas.rheinhardt at outlook.com> wrote:
>>>
>>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at outlook.com>
>>>> ---
>>>>   libavutil/avstring.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/libavutil/avstring.c b/libavutil/avstring.c
>>>> index 2071dd36a5..8702fe0455 100644
>>>> --- a/libavutil/avstring.c
>>>> +++ b/libavutil/avstring.c
>>>> @@ -299,7 +299,7 @@ char *av_append_path_component(const char *path,
>>>> const
>>>> char *component)
>>>>       char *fullpath;
>>>>
>>>>       if (!path)
>>>> -        return av_strdup(component);
>>>> +        return component ? av_strdup(component) : NULL;
>>>>       if (!component)
>>>>           return av_strdup(path);
>>>>
>>>
>>> isn't this what av_strdup already does?
>>
>> It's not documented to do so. It could also decide to treat
>> av_strdup(NULL) as av_strdup("").
> 
> Even if undocumented, that would be an unexpected change in behavior.
> I'm with Vittorio, we should mention the actual behavior in the doxy and
> make it official.

The problematic thing is that this behaviour is also mostly insane. Or
rather: If you care to distinguish "returns NULL due to allocation
failure" and "returns NULL due to NULL src", then you have to check src
for NULL anyway and then you could also just have checked before calling
av_strdup and avoided it in case src is NULL. This code here is the only
example I found for which the current behaviour is actually reasonable.

- Andreas