[FFmpeg-devel] [PATCH] avformat/libsrt: Decode URL parameter strings

["zhilizhao(赵志立)"]

> On Aug 11, 2023, at 00:13, Armin Hasitzka <armin at grabyo.com> wrote:
> 
> Hi again,
> 
> we found this the other day when using a stream ID containing "%2F",
> expecting this to be resolved to "/". While "%2F" could obviously be sent
> decoded, "&" (decoded) would currently end the value and not be used, "+"
> (decoded) would be overwritten with " ", and "=" (decoded) could cause
> entirely unexpected outcomes (albeit all these characters being allowed by
> SRT for various string inputs).
> 
> As for changing `av_strndup` to `ff_urldecode` (which removes a length
> check); I don't think that this particular length check added any
> protection (`av_find_info_tag` adds a trailing `\0` if found). This
> thinking is also evident at the two other places where `ff_urldecode`
> replaced `av_strdup`.
> 
> It would be amazing if we could get this merged upstream :)
> 

Thanks for the contribution, but the issue is kind of complex.

1. The format of streamid isn’t take URL into consideration

"#!::u=admin,r=foo”

2. Obviously some implementation and usecases depend on these fields to
be passed directly without URL encoding/decoding.

The same issue has been filed to srt again and again, ref.

https://github.com/Haivision/srt/issues/2749

and #1871, #1173, #2015.

I think we shouldn’t put these fields into URL at the first place.
Now doing any change, even if it’s technically correct, will make
regression issues.

I have no idea to improve the code. But it’s easy to solve from the
user’s point of view: don’t set these fields via URL, just use options.
These is no implementation dependent behavior with options.

> ---
>  libavformat/libsrt.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/libavformat/libsrt.c b/libavformat/libsrt.c
> index cd8f5b1e7d..8986616334 100644
> --- a/libavformat/libsrt.c
> +++ b/libavformat/libsrt.c
> @@ -32,6 +32,7 @@
>  #include "network.h"
>  #include "os_support.h"
>  #include "url.h"
> +#include "urldecode.h"
>  
>  /* This is for MPEG-TS and it's a default SRTO_PAYLOADSIZE for SRTT_LIVE (8 TS packets) */
>  #ifndef SRT_LIVE_DEFAULT_PAYLOAD_SIZE
> @@ -547,7 +548,7 @@ static int libsrt_open(URLContext *h, const char *uri, int flags)
>          }
>          if (av_find_info_tag(buf, sizeof(buf), "passphrase", p)) {
>              av_freep(&s->passphrase);
> -            s->passphrase = av_strndup(buf, strlen(buf));
> +            s->passphrase = ff_urldecode(buf, 0);
>          }
>  #if SRT_VERSION_VALUE >= 0x010302
>          if (av_find_info_tag(buf, sizeof(buf), "enforced_encryption", p)) {
> @@ -632,7 +633,7 @@ static int libsrt_open(URLContext *h, const char *uri, int flags)
>          }
>          if (av_find_info_tag(buf, sizeof(buf), "streamid", p)) {
>              av_freep(&s->streamid);
> -            s->streamid = av_strdup(buf);
> +            s->streamid = ff_urldecode(buf, 0);
>              if (!s->streamid) {
>                  ret = AVERROR(ENOMEM);
>                  goto err;
> @@ -640,7 +641,7 @@ static int libsrt_open(URLContext *h, const char *uri, int flags)
>          }
>          if (av_find_info_tag(buf, sizeof(buf), "smoother", p)) {
>              av_freep(&s->smoother);
> -            s->smoother = av_strdup(buf);
> +            s->smoother = ff_urldecode(buf, 0);
>              if(!s->smoother) {
>                  ret = AVERROR(ENOMEM);
>                  goto err;
> -- 
> 2.41.0
> 
>