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

[Rashad Tatum <tatum.rashad@gmail.com>]

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@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@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@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@ffmpeg.org
>>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>
>>> To unsubscribe, visit link above, or email
>>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>>>
>>
_______________________________________________
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".