Re: [PATCH] vbscript: Implemented String

[Piotr Caban]

On 10/19/14 17:44, Shuai Meng wrote:
> - newstr = SysAllocStringLen(NULL, 1023);

You need to allocate correct length string. First argument should be 
used here. You're also leaking the string in error-handling paths.
> -
> - switch(V_VT(arg + 1)) {
> - case VT_NULL:
> -
>    return MAKE_VBSERROR(VBSE_ILLEGAL_NULL_USE);
> - case VT_BSTR:
> -
>    str = V_BSTR(arg + 1);
> -
>    break;
> - case  VT_ARRAY|VT_BYREF|VT_VARIANT:
> -
>    return DISP_E_TYPEMISMATCH;
> - default:
> -
>    hres = to_short(arg + 1, &tmp);
> -
>    if(FAILED(hres))
> -
>        return hres;
> -
>    str[0] = (char)tmp;

Please add a test with second argument larger then 256. It would be also 
interesting to see a test with second argument being BSTR with first 
character out of ASCII range. Please also check what happens if 
V_BSTR(arg+1)==NULL.
> - hres = to_short(arg, &len);
> - if(FAILED(hres))
> -
>    return hres;
> - if(len <  0)
> -
>    return MAKE_VBSERROR(VBSE_ILLEGAL_FUNC_CALL);
> - else if(len == 0)
> -
>    newstr = '\0';
> - else if(len > 1023)
> -
>    len = 1023;

I guess that first argument is probably an integer. The length of 
produced string is probably also not limited to 1023 characters.

Thanks,
Piotr

[PATCH] vbscript: Implemented String

[Shuai Meng]

2014-10-20 17:00 GMT+08:00 Piotr Caban :
> On 10/19/14 17:44, Shuai Meng wrote:
> > - newstr = SysAllocStringLen(NULL, 1023);
>
> You need to allocate correct length string. First argument should be used
> here. You're also leaking the string in error-handling paths.

You mean I should replace 1023 with the final number of first argument?
Seems good.
> -
> > - switch(V_VT(arg + 1)) {
> > - case VT_NULL:
> > -
> >    return MAKE_VBSERROR(VBSE_ILLEGAL_NULL_USE);
> > - case VT_BSTR:
> > -
> >    str = V_BSTR(arg + 1);
> > -
> >    break;
> > - case  VT_ARRAY|VT_BYREF|VT_VARIANT:
> > -
> >    return DISP_E_TYPEMISMATCH;
> > - default:
> > -
> >    hres = to_short(arg + 1, &tmp);
> > -
> >    if(FAILED(hres))
> > -
> >        return hres;
> > -
> >    str[0] = (char)tmp;
>
> Please add a test with second argument larger then 256. It would be also
> interesting to see a test with second argument being BSTR with first
> character out of ASCII range.

In fact it is hard to check what String returns when character is larger
than 256 even below 256. For example, String(1,0) returns an empty string
on xp, but Eval(String(1,0) = "") will return false, we all know 0
represents '\0' in ASCII which is not a print character. This is the same
situation when the second argument is negative number.
> Please also check what happens if V_BSTR(arg+1)==NULL.

I have added such test:
Call testStringError(2, Null, 94)
> - hres = to_short(arg, &len);
> > - if(FAILED(hres))
> > -
> >    return hres;
> > - if(len <  0)
> > -
> >    return MAKE_VBSERROR(VBSE_ILLEGAL_FUNC_CALL);
> > - else if(len == 0)
> > -
> >    newstr = '\0';
> > - else if(len > 1023)
> > -
> >    len = 1023;
>
> I guess that first argument is probably an integer.

Do you mean we don't need  to_short(arg, &len)?
> The length of produced string is probably also not limited to 1023
> characters.

I have answered this question in the reply to Nikolay~
> Thanks,
> Piotr
>
> Thanks for commenting Piotr.