Re: [PATCH] jscript: Implement decodeURIComponent

[Jacek Caban]

Hi Andrew,

Andrew Eikum wrote:
> dlls/jscript/global.c     |  104 
> ++++++++++++++++++++++++++++++++++++++++++++-
>  dlls/jscript/tests/api.js |   20 +++++++++
>  2 files changed, 122 insertions(+), 2 deletions(-)

+            while(*ptr == '%') {
+                if(hex_to_int(*(ptr+1)) < 0 || hex_to_int(*(ptr+2)) < 0) {
+                    WARN("Invalid hex sequence: 0x%x 0x%x\n", *(ptr+1), *(ptr+2));
+                    SysFreeString(str);
+                    return E_FAIL;

URIError should be thrown here. If it's not, then you should put FIXME here. Same for other places where you return E_FAIL.

+                }
+                ptr += 3;
+                ++buf_len;
+            }
+            octets = heap_alloc(buf_len);

You don't check allocation failure and leak octets here. You may avoid dynamic allocation at all by processing one utf8 char at the time. Then you know that it's not longer than 4 bytes. Ecma specification describes it well, although you may simplify the algorithm by checking number of bytes from the first byte, fetching bytes to the array and then use MultiByteToWideChar for conversion to UTF-16 and validation.

Thanks,
    Jacek

[PATCH] jscript: Implement decodeURIComponent

[Andrew Eikum]

Jacek Caban wrote:
> Hi Andrew,
>
> Andrew Eikum wrote:
> > dlls/jscript/global.c     |  104 
> > ++++++++++++++++++++++++++++++++++++++++++++-
> >  dlls/jscript/tests/api.js |   20 +++++++++
> >  2 files changed, 122 insertions(+), 2 deletions(-)
> -
>        while(*ptr == '%') {
> -
>            if(hex_to_int(*(ptr+1)) < 0 || hex_to_int(*(ptr+2)) < 0) {
> -
>                WARN("Invalid hex sequence: 0x%x 0x%x\n", *(ptr+1), 
>
> *(ptr+2));
> -
>                SysFreeString(str);
> -
>                return E_FAIL;
>
> URIError should be thrown here. If it's not, then you should put FIXME 
> here. Same for other places where you return E_FAIL.
> -
>            }
> -
>            ptr += 3;
> -
>            ++buf_len;
> -
>        }
> -
>        octets = heap_alloc(buf_len);
>
>
> You don't check allocation failure and leak octets here. You may avoid 
> dynamic allocation at all by processing one utf8 char at the time. Then 
> you know that it's not longer than 4 bytes. Ecma specification describes 
> it well, although you may simplify the algorithm by checking number of 
> bytes from the first byte, fetching bytes to the array and then use 
> MultiByteToWideChar for conversion to UTF-16 and validation.
>
> Thanks,
>     Jacek

Thanks for looking it over, Jacek.  I've resent with your suggestions.

Andrew