Hello, those two patches should fix Debian #654665 [0]. [0] http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=654665 Sebastian
My URL looks the following: rtmp://vod.daserste.de:8080/ardfs/mp4:videoportal/mediathek/Plusminus/c_320000/321891/format406703.mp4?&sen=Plusminus&clip=%27Al… and the parser creates interprets this as: app : 'ardfs/mp4:videoportal/mediathek' path: 'mp4:Plusminus/c_320000/321891/format406703?&sen=Plusminus&cl… The application is too long, as it is only "ardfs" and as a result the playpath gets wrong. I have no idea why it looks for so many slashes because the application should end after the first slash before the mp4: string. Commit f3e3e6b5 ("Look for a fourth slash when splitting the url into app+playpath") added even another one and I have no idea why. For the time now I ignore all slashes past "mp[34]:" since this should be this should mark the end the application and the start of the playpath. Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc> --- librtmp/parseurl.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/librtmp/parseurl.c b/librtmp/parseurl.c index 646c70c..95562d4 100644 --- a/librtmp/parseurl.c +++ b/librtmp/parseurl.c @@ -34,6 +34,7 @@ int RTMP_ParseURL(const char *url, int *protocol, AVal *host, unsigned int *port AVal *playpath, AVal *app) { char *p, *end, *col, *ques, *slash; + char *mp_str; RTMP_Log(RTMP_LOGDEBUG, "Parsing..."); @@ -130,6 +131,9 @@ parsehost: } p = slash+1; + mp_str = strstr(p, "mp4:"); + if (!mp_str) + mp_str = strstr(p, "mp3:"); { /* parse application * @@ -141,10 +145,16 @@ parsehost: int applen, appnamelen; slash2 = strchr(p, '/'); - if(slash2) + if(slash2) { slash3 = strchr(slash2+1, '/'); - if(slash3) + if (mp_str && slash3 > mp_str) + slash3 = NULL; + } + if(slash3) { slash4 = strchr(slash3+1, '/'); + if (mp_str && slash4 > mp_str) + slash4 = NULL; + } applen = end-p; /* ondemand, pass all parameters as app */ appnamelen = applen; /* ondemand length */ -- 1.7.10.4
Part of my URL goes like this: mp;clip=%27Alle+Beitr%E4ge+-+di The code now converts %27 into ' and %e4 into �. The first conversion of %27 seems fine but the second isn't since it is not an ascii character and this on has to be encoded with the % and sent as is. I believe the only reason here is to keep URL human readable where possible. Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc> --- librtmp/parseurl.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/librtmp/parseurl.c b/librtmp/parseurl.c index 95562d4..c68da2e 100644 --- a/librtmp/parseurl.c +++ b/librtmp/parseurl.c @@ -284,9 +284,14 @@ void RTMP_ParsePlaypath(AVal *in, AVal *out) { if (*p == '%') { unsigned int c; sscanf(p+1, "%02x", &c); - *destptr++ = c; - pplen -= 3; - p += 3; + if (isascii(c)) { + *destptr++ = c; + pplen -= 3; + p += 3; + } else { + *destptr++ = *p++; + pplen--; + } } else { *destptr++ = *p++; pplen--; -- 1.7.10.4
Sebastian Andrzej Siewior <sebastian@breakpoint.cc> wrote:
Part of my URL goes like this: mp;clip=%27Alle+Beitr%E4ge+-+di
The code now converts %27 into ' and %e4 into �. The first conversion of %27 seems fine but the second isn't since it is not an ascii character and this on has to be encoded with the % and sent as is. I believe the only reason here is to keep URL human readable where possible.
Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc> --- librtmp/parseurl.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/librtmp/parseurl.c b/librtmp/parseurl.c index 95562d4..c68da2e 100644 --- a/librtmp/parseurl.c +++ b/librtmp/parseurl.c @@ -284,9 +284,14 @@ void RTMP_ParsePlaypath(AVal *in, AVal *out) { if (*p == '%') { unsigned int c; sscanf(p+1, "%02x", &c); - *destptr++ = c; - pplen -= 3; - p += 3; + if (isascii(c)) { + *destptr++ = c; + pplen -= 3; + p += 3; + } else { + *destptr++ = *p++; + pplen--; + } } else { *destptr++ = *p++; pplen--;
If you are going to do that, you'd better also not decode the ascii character '%'.
On Fri, 24 May 2013, Sebastian Andrzej Siewior wrote:
My URL looks the following: rtmp://vod.daserste.de:8080/ardfs/mp4:videoportal/mediathek/Plusminus/c_320000/321891/format406703.mp4?&sen=Plusminus&clip=%27Al…
and the parser creates interprets this as:
app : 'ardfs/mp4:videoportal/mediathek' path: 'mp4:Plusminus/c_320000/321891/format406703?&sen=Plusminus&cl…
The application is too long, as it is only "ardfs" and as a result the playpath gets wrong. I have no idea why it looks for so many slashes because the application should end after the first slash before the mp4: string. Commit f3e3e6b5 ("Look for a fourth slash when splitting the url into app+playpath") added even another one and I have no idea why.
Well, the reason is pretty obvious - given a url like rtmp://server/a/b/c/d/e/f, there's nothing that says which part is the app and which is the playpath. So far the assumption has been to use most of the path as app, and the rest as playpath (only taking a fixed number of path segments probably because the implementation was lazy or so). And changing it back would just break one case and fix another one. The fact that you can use mp4: or mp3: as indication of where the playpath probably start is a better heuristic than what the lazy implementation so far has done, and the patch looks good to me. However, in general one can't expect the heuristic to handle every single case - if in doubt one can always pass them separately. // Martin
On Sat, May 25, 2013 at 11:20:23AM +1200, David Sainty wrote:
If you are going to do that, you'd better also not decode the ascii character '%'.
Hmm. What about characters in the reserved set as per RFC3986 chapter 2.2 [0]: : / ? # [ ] @ ! $ & ' ( ) * + , ; = those should be skipped, too right? So is would be isalnum() fit better here? Or let me ask differently: Why do we do this at all? Why not send the request percent encoded if this is the URL the user got? [0] http://tools.ietf.org/html/rfc3986#section-2.2 Sebastian
On Sat, May 25, 2013 at 05:35:44PM +0300, Martin Storsjö wrote:
rtmp://vod.daserste.de:8080/ardfs/mp4:videoportal/mediathek/Plusminus/c_320000/321891/format406703.mp4?&sen=Plusminus&clip=%27Al… app : 'ardfs/mp4:videoportal/mediathek' path: 'mp4:Plusminus/c_320000/321891/format406703?&sen=Plusminus&cl… Well, the reason is pretty obvious - given a url like rtmp://server/a/b/c/d/e/f, there's nothing that says which part is
On Fri, 24 May 2013, Sebastian Andrzej Siewior wrote: the app and which is the playpath. So far the assumption has been to use most of the path as app, and the rest as playpath (only taking a fixed number of path segments probably because the implementation was lazy or so). And changing it back would just break one case and fix another one.
I was under the assumption there is a standard but now I learned that this is not the case. So it is up to the server owner whatever he decides.
The fact that you can use mp4: or mp3: as indication of where the playpath probably start is a better heuristic than what the lazy implementation so far has done, and the patch looks good to me. However, in general one can't expect the heuristic to handle every single case - if in doubt one can always pass them separately.
Would it be okay to add a little testsuite with a bunch of rtmp urls which checks if the app and path is parsed properly?
// Martin
Sebastian
Sebastian Andrzej Siewior wrote:
On Sat, May 25, 2013 at 05:35:44PM +0300, Martin Storsjö wrote:
rtmp://vod.daserste.de:8080/ardfs/mp4:videoportal/mediathek/Plusminus/c_320000/321891/format406703.mp4?&sen=Plusminus&clip=%27Al… app : 'ardfs/mp4:videoportal/mediathek' path: 'mp4:Plusminus/c_320000/321891/format406703?&sen=Plusminus&cl… Well, the reason is pretty obvious - given a url like rtmp://server/a/b/c/d/e/f, there's nothing that says which part is
On Fri, 24 May 2013, Sebastian Andrzej Siewior wrote: the app and which is the playpath. So far the assumption has been to use most of the path as app, and the rest as playpath (only taking a fixed number of path segments probably because the implementation was lazy or so). And changing it back would just break one case and fix another one.
I was under the assumption there is a standard but now I learned that this is not the case. So it is up to the server owner whatever he decides.
The fact that you can use mp4: or mp3: as indication of where the playpath probably start is a better heuristic than what the lazy implementation so far has done, and the patch looks good to me. However, in general one can't expect the heuristic to handle every single case - if in doubt one can always pass them separately.
Would it be okay to add a little testsuite with a bunch of rtmp urls which checks if the app and path is parsed properly?
It is totally pointless. Users should just learn to explicitly pass the app and playpath and not rely on the builtin parser at all. In fact, I've a mind to revert all of the recent parsing patches back to my last version. If your particular usage fails to parse correctly, then you should manually retry. It's a waste of developer time to focus on this. It's not a problem for real Flash clients because each SWF app is custom tailored to the service it's talking to, so they all already know how to parse their own URLs. Code like librtmp will never have this kind of knowledge due to its generality. Going forward, we should, as a matter of course, discard all patches to the parsing code as a waste of time.
Sebastian Andrzej Siewior wrote:
On Sat, May 25, 2013 at 11:20:23AM +1200, David Sainty wrote:
If you are going to do that, you'd better also not decode the ascii character '%'.
Hmm. What about characters in the reserved set as per RFC3986 chapter 2.2 [0]: : / ? # [ ] @ ! $ & ' ( ) * + , ; =
those should be skipped, too right? So is would be isalnum() fit better here? Or let me ask differently: Why do we do this at all? Why not send the request percent encoded if this is the URL the user got?
99% of the behaviors encoded in librtmp were developed for and tested against the BBC iPlayer and Hulu Adobe Media servers. Everything in here was needed for them at some point in time.
On Sat, May 25, 2013 at 11:25 AM, Howard Chu wrote:
It is totally pointless. Users should just learn to explicitly pass the app and playpath and not rely on the builtin parser at all. In fact, I've a mind to revert all of the recent parsing patches back to my last version. If your particular usage fails to parse correctly, then you should manually retry.
I see your point, the endless slash parsing should go away I agree. However with the "mp3:" "mp4:" we have a clear indicator of the start of the playpath, so why not use it? I say use the "mp3:" "mp4:" as the start of the playpath if found, otherwise assume "app" stops at first slash. I can write patch for this if agreeable.
Steven Penny wrote:
On Sat, May 25, 2013 at 11:25 AM, Howard Chu wrote:
It is totally pointless. Users should just learn to explicitly pass the app and playpath and not rely on the builtin parser at all. In fact, I've a mind to revert all of the recent parsing patches back to my last version. If your particular usage fails to parse correctly, then you should manually retry.
I see your point, the endless slash parsing should go away I agree. However with the "mp3:" "mp4:" we have a clear indicator of the start of the playpath, so why not use it?
I say use the "mp3:" "mp4:" as the start of the playpath if found, otherwise assume "app" stops at first slash. I can write patch for this if agreeable.
It sounds OK at first, but... We really have no idea what other valid filetypes may show up there. I'm tempted to try "/[any-3-or-4-chars]:" as the pattern instead. Then again, perhaps mp3/mp4 is enough. At this point I think Adobe has stopped investing in RTMP and gone on to other protocols. Dunno.
participants (5)
-
David Sainty -
Howard Chu -
Martin Storsjö -
Sebastian Andrzej Siewior -
Steven Penny