[PATCH] msxml3: all string passed to IXMLDOMDocument_load() need to be URL-unescaped

[Damjan Jovanovic]

msxml3 allows URL escape sequences even for C:\ style paths.
eg. C:\Program%20Files\...

Signed-off-by: Damjan Jovanovic <damjan.jov(a)gmail.com>
---
 dlls/msxml3/bsc.c          |  8 ++++++--
 dlls/msxml3/tests/domdoc.c | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 2 deletions(-)

[Nikolay Sivov]

Do we need to unescape if it's not a file:// url?I was hoping we could 
find some IUri/path API flags to do what we need for us.

Another concern is path/url max lengths are used seemingly randomly, and 
it's not a problem with your change, but with existing code.I think 
asking for required length and allocating it is better.
> +    /* Regular local path with some URL encoded characters. */
> +    strcpy(path2, path);
> +    n = strlen(path2);
> +    path2[n-1] = '%';
> +    path2[n] = '6';
> +    path2[n+1] = 'C';
> +    path2[n+2] = '\0';   /* C:\path\to\winetest.xm%6C */
> +    test_doc_load_from_path(doc, path2);
> Could you make this more readable? Maybe strcat-ing escaped file name as 
> string literal instead.
> +    /* Regular local path with all URL encoded characters. */
> +    percent_path = HeapAlloc(GetProcessHeap(), 0, 3*n + 1);
> +    for (i = 0; i < n; i++)
> +    {
> +        static char hex_tab[] = "0123456789ABCDEF";
> +        percent_path[3*i] = '%';
> +        percent_path[3*i + 1] = hex_tab[path[i] >> 4];
> +        percent_path[3*i + 2] = hex_tab[path[i] & 0xF];
> +    }
> +    percent_path[3*n] = '\0';
> +    test_doc_load_from_path(doc, percent_path);
> +    HeapFree(GetProcessHeap(), 0, percent_path);
> Couple of cases would be enough, like you did earlier for "l" -> %6c, 
> and another one for " " -> %20. Space is actually not tested by this 
> patch, and that's what application is using. If you still want to encode 
> it entirely, please add a helper function.

[Damjan Jovanovic]

On Thu, Dec 5, 2019 at 1:45 PM Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
> Do we need to unescape if it's not a file:// url? I was hoping we could
> find some IUri/path API flags to do what we need for us.

The CreateUri() function called at the end of create_uri() does its own
unescaping, which is why I didn't pass it the unescaped URL, but left it
with the original URL. My unescaping is done for the benefit of the earlier
function, PathIsURLW(), which doesn't unescape internally, and for
PathSearchAndQualifyW() and UrlCreateFromPathW(), which require paths not
URLs, and paths are always unescaped.
> Another concern is path/url max lengths are used seemingly randomly, and
> it's not a problem with your change, but with existing code. I think
> asking for required length and allocating it is better.

Sure.
> +    /* Regular local path with some URL encoded characters. */
> +    strcpy(path2, path);
> +    n = strlen(path2);
> +    path2[n-1] = '%';
> +    path2[n] = '6';
> +    path2[n+1] = 'C';
> +    path2[n+2] = '\0';   /* C:\path\to\winetest.xm%6C */
> +    test_doc_load_from_path(doc, path2);
>
> Could you make this more readable? Maybe strcat-ing escaped file name as
> string literal instead.

Will do.
> +    /* Regular local path with all URL encoded characters. */
> +    percent_path = HeapAlloc(GetProcessHeap(), 0, 3*n + 1);
> +    for (i = 0; i < n; i++)
> +    {
> +        static char hex_tab[] = "0123456789ABCDEF";
> +        percent_path[3*i] = '%';
> +        percent_path[3*i + 1] = hex_tab[path[i] >> 4];
> +        percent_path[3*i + 2] = hex_tab[path[i] & 0xF];
> +    }
> +    percent_path[3*n] = '\0';
> +    test_doc_load_from_path(doc, percent_path);
> +    HeapFree(GetProcessHeap(), 0, percent_path);
>
> Couple of cases would be enough, like you did earlier for "l" -> %6c, and
> another one for " " -> %20. Space is actually not tested by this patch, and
> that's what application is using. If you still want to encode it entirely,
> please add a helper function.

Agreed. It would actually be good to test with both a space and %20.