[FFmpeg-devel] [PATCH v5 2/3] libavformat/rtsp: Free memory allocated for temporary variables while processing sdp info

[Rashad Tatum]

I'll submit a separate patch for this issue

On Wed, Feb 26, 2025, 8:18 AM Rashad Tatum <tatum.rashad at gmail.com> wrote:

> So, perhaps sdp_parse_line should not be void and should propagate an
> error code up that should be returned by ff_sdp_parse.
>
> On Wed, Feb 26, 2025, 8:16 AM Rashad Tatum <tatum.rashad at gmail.com> wrote:
>
>> In other parts of the ffmpeg code base where the return type is not void
>> for the function, NULL is returned and then the caller typically returns
>> AVERROR.
>>
>> Strangely, sdp_parse_line is void and it seems like ff_sdp_parse always
>> returns 0, despite the caller (sdp_read_header) attempting to handle any
>> potential returned error codes.
>>
>> On Wed, Feb 26, 2025, 7:52 AM Rashad Tatum <tatum.rashad at gmail.com>
>> wrote:
>>
>>> As a side note,  I think the failure of the rtsp_src memory allocation
>>> should also log an error so that the user is aware of why the rtsp stream
>>> failed to play. However, I think this change should be part of separate
>>> patch series.
>>>
>>> On Wed, Feb 26, 2025, 7:14 AM Rashad Tatum <tatum.rashad at gmail.com>
>>> wrote:
>>>
>>>> Ok. I'll move this and the third patch to one commit/patch and also
>>>> change the condition when rtsp_src can't be allocated to also free
>>>> dest_addr. Good catch.
>>>>
>>>>
>>>> On Wed, Feb 26, 2025, 3:53 AM Andreas Rheinhardt <
>>>> andreas.rheinhardt at outlook.com> wrote:
>>>>
>>>>> Rashad Tatum:
>>>>> > ---
>>>>> >  libavformat/rtsp.c | 2 ++
>>>>> >  1 file changed, 2 insertions(+)
>>>>> >
>>>>> > diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c
>>>>> > index 0c65f8d1a4..ac17717195 100644
>>>>> > --- a/libavformat/rtsp.c
>>>>> > +++ b/libavformat/rtsp.c
>>>>> > @@ -478,6 +478,7 @@ static void sdp_parse_line(AVFormatContext *s,
>>>>> SDPParseState *s1,
>>>>> >                                                sdp_ip_str);
>>>>> >
>>>>> >          }
>>>>> > +        av_freep(&sdp_ip_str);
>>>>> >          break;
>>>>> >      case 's':
>>>>> >          av_dict_set(&s->metadata, "title", p, 0);
>>>>> > @@ -702,6 +703,7 @@ static void sdp_parse_line(AVFormatContext *s,
>>>>> SDPParseState *s1,
>>>>> >                      }
>>>>> >                  }
>>>>> >              }
>>>>> > +            av_freep(&dest_addr);
>>>>> >          } else {
>>>>> >              if (rt->server_type == RTSP_SERVER_WMS)
>>>>> >                  ff_wms_parse_sdp_a_line(s, p);
>>>>>
>>>>> Your first patch introduces a memleak; the stuff you allocated there
>>>>> should be freed in the patch, not fixed up in a later patch.
>>>>> (Also note that there must not be a leak in any error path, which is
>>>>> not
>>>>> true when your patchset is applied: dest_addr leaks when rtsp_src can't
>>>>> be allocated.)
>>>>>
>>>>> - Andreas
>>>>>
>>>>> _______________________________________________
>>>>> ffmpeg-devel mailing list
>>>>> ffmpeg-devel at ffmpeg.org
>>>>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>>>
>>>>> To unsubscribe, visit link above, or email
>>>>> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>>>>>
>>>>