I think accepting URL params in first place isn't inherently bad (depending on the use case, it might make certain interactions easier).
I expected regressions to be brought up and just want to point out that, while absolutely possible, actual regressions will likely be fairly rare: - any "unencoded" characters will continue to work (eg "/" passed in will continue to be "/") - usage of "+" and "&" will cause unintended behaviour already ("=" _might_ cause unintended behaviour today) - the main regression comes from "%.." substrings that are _not_ meant to be url URL encoded but happen to be that and will now be turned into something else Because of this, I did not expect this patch to make it into any patch or minor release. We were hoping though, that this would be a contender for the next major one. On Fri, 11 Aug 2023 at 03:55, "zhilizhao(赵志立)" <quinkbl...@foxmail.com> wrote: > > > > On Aug 11, 2023, at 00:13, Armin Hasitzka <ar...@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 > > > > > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@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-requ...@ffmpeg.org with subject "unsubscribe".