GNOME Bugzilla – Bug 730830
hlsdemux: Remove \" sign from decryption KEY url does not always work
Last modified: 2014-06-06 10:15:16 UTC
Created attachment 277304 [details] [review] Proposition of solution of this bug When we are parsing decryptin key for HLS stream from manifest file parsing line has syntax like below one: #EXT-X-KEY:METHOD=AES-128,URI="https://priv.example.com/key.php?r=52" Implementation of parsing such line in current version of hlsdemux works in most situations. It removes \" signes form key URI and returns key as http or https URI (in current situation it should return: https://priv.example.com/key.php?r=52 However, I met manifest files which was not parsed properly because of white characters (or some other characters) at the end of above line. In such situations I received URL key still containing \" character at the end (in current situation it could be: https://priv.example.com/key.php?r=52" Notice \" sign still being at the end of URL after parsing I think it is bug. therefore I propose solution in the attached patch which worked for me in the above situations and provided me proper key URL for all manifest files which I tested. SOLUTION: my proposition of solution is in the attached file
Review of attachment 277304 [details] [review]: ::: ext/hls/m3u8.c @@ +424,3 @@ /* handle the \"uri\" case */ + /* there are sometimes situations where we have white signs + * before or after \" sign of URL therefore we are using for loops Maybe we should just remove spaces then? g_ascii_isspace() or something @@ +426,3 @@ + * before or after \" sign of URL therefore we are using for loops + * in order to remove first and last \" sign from decryption key URL */ + for(guint i = 0; i < len; ++i){ Declare the variable outside the for loop at the beginning of the block. We don't allow C99 yet :) @@ +428,3 @@ + for(guint i = 0; i < len; ++i){ + if (uri[len - i] == '"'){ + uri[len - i] = '\0'; if i == 0 you write to uri[len], which is one element after the end @@ +432,3 @@ + } + } + for(guint i = 0; i < len; ++i){ variable declaration
Created attachment 277314 [details] [review] Solution patch version 2
I put another patch above according to your suggestions
Review of attachment 277314 [details] [review]: ::: ext/hls/m3u8.c @@ +490,3 @@ + if(!is_init_quotation_mark){ + //found initializing quotation mark of URL + key += (i+1); Here you change key, over which you continue to iterate. That's probably not a good idea, you can easily go after the end of the string and also skip over some part of the key URI @@ +497,3 @@ + break; + } + } I would say that it should fail if opening but no closing " are found
Created attachment 277319 [details] [review] Solution patch version 3
Review of attachment 277319 [details] [review]: ::: ext/hls/m3u8.c @@ +491,3 @@ + if(!is_init_quotation_mark){ + //found initializing quotation mark of URL + key += (++i); This is still wrong for the same reasons as before :) Also maybe just use strchr() here to find the first " and then the second " from there
Thanks for your suggestions, Sebastian. I am including patch containing solution based on strchr().
Created attachment 277363 [details] [review] Solution patch version 4
commit 5ca7684b7da50803376c2273f6070af84f64ffdc Author: Damian Ziobro <damian@xmementoit.com> Date: Wed May 28 09:18:49 2014 +0100 hlsdemux: Make parsing of "-quoted key URIs more resilient https://bugzilla.gnome.org/show_bug.cgi?id=730830
Created attachment 277824 [details] [review] Improve removing quotation marks from URIs
Sebastian, I improved removing quotation marks from URIs: - put it to separate function - apply it to I-frame-based stream URIs (I forgot about it previuosly) Patch submitted above.
Review of attachment 277824 [details] [review]: Please put your real name into your GIT settings, so it shows up in the From line ::: ext/hls/m3u8.c @@ +243,3 @@ +static gboolean +remove_quotation_marks_around_uri (gchar * uri) unquote_string() maybe? Not really URI related. It converts any "blablabla" string to the equivalent without quotes... inplace @@ +247,3 @@ + gchar *key_ret; + + /* handle the \"key\" case * not really key specific anymore @@ +261,3 @@ + } else { + GST_WARNING + ("Decryption key URL parsing - cannot find finalizing quotation mark"); Not always correct if used for the I-Frame lists @@ +265,3 @@ + } + } + return TRUE; You have to return uri here. You might change the pointer to skip over the initial stuff until the initial "
Created attachment 277892 [details] [review] Another improvements to patch Updates of patch according to Sebastian's suggestions.
commit be28578942b93d639cb5da0633a1c8524ce9784f Author: Damian Ziobro <damian@xmementoit.com> Date: Wed Jun 4 18:34:44 2014 +0100 hlsdemux: Improve parsing quoted key URIs and apply it for I-frame-based stream URI https://bugzilla.gnome.org/show_bug.cgi?id=730830