Re: [PATCH] vbscript: Implemented String

[Nikolay Sivov]

> - str = SysAllocStringLen(NULL, 1023);
> - newstr = SysAllocStringLen(NULL, 1023);

Where this length comes from?
> - 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;
> -
>    break;
> - }

You only need first character, right? Then why do you need a full BSTR 
pointer in VT_BSTR case? And assigning it
to 'str' you leak a previously allocated buffer. Why cast to (char)tmp?
> - else if(len == 0)
> -
>    newstr = '\0';

Same way you're losing pointer to allocated buffer.

[PATCH] vbscript: Implemented String

[Shuai Meng]

Thank you very much for commenting on this patch.

2014-10-20 0:06 GMT+08:00 Nikolay Sivov :
> - str = SysAllocStringLen(NULL, 1023);
> > - newstr = SysAllocStringLen(NULL, 1023);
>
> Where this length comes from?
> Well, I tested String on windows xp, and found that 1023 was the limit,
> when given a number bigger than that, the output kept the length of 1023.
> > - 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;
> > -
> >    break;
> > - }
>
> You only need first character, right? Then why do you need a full BSTR
> pointer in VT_BSTR case? And assigning it
> to 'str' you leak a previously allocated buffer.

So how do I get the first character of (arg + 1)?  How about this: str[0] =
* V_BSTR(arg + 1)
In fact I don't quite understand how SysAllocStringLen work, but I see it
is used in the former function, so I think maybe it is necessary.
> Why cast to (char)tmp?

I think the type of str[0] is WCHAR, and tmp is an integer, shouldn't we
make a cast?
> - else if(len == 0)
> > -
> >    newstr = '\0';
>
> Same way you're losing pointer to allocated buffer.

[PATCH] vbscript: Implemented String

[Nikolay Sivov]

On 10/20/2014 19:58, Shuai Meng wrote:
> Thank you very much for commenting on this patch.
>
> 2014-10-20 0:06 GMT+08:00 Nikolay Sivov <[e-mail] 
> >:
>     +    str = SysAllocStringLen(NULL, 1023);
>     +    newstr = SysAllocStringLen(NULL, 1023);
>
> Where this length comes from?
> Well, I tested String on windows xp, and found that 1023 was the
> limit, when given a number bigger than that, the output kept the
> length of 1023.

Can you add this too as a test? In some compact way if possible.
>     +    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;
>     +        break;
>     +    }
>
> You only need first character, right? Then why do you need a full
> BSTR pointer in VT_BSTR case? And assigning it
> to 'str' you leak a previously allocated buffer. 
>
> So how do I get the first character of (arg + 1)?  How about this: 
> str[0] = * V_BSTR(arg + 1)
> In fact I don't quite understand how SysAllocStringLen work, but I see 
> it is used in the former function, so I think maybe it is necessary.

What I don't get is why do you allocate 'str' at all, it should be just 
'WCHAR ch;' variable.
> Why cast to (char)tmp?
>
> I think the type of str[0] is WCHAR, and tmp is an integer, shouldn't 
> we make a cast?

WCHAR is 'unsigned short'. And it feels like it needs more tests for 
other variant types.
>     +    else if(len == 0)
>     +        newstr = '\0';
>
> Same way you're losing pointer to allocated buffer.

[PATCH] vbscript: Implemented String

[Shuai Meng]

Thanks for commenting.

2014-10-21 0:20 GMT+08:00 Nikolay Sivov :
> On 10/20/2014 19:58, Shuai Meng wrote:
>
> Thank you very much for commenting on this patch.
>
> 2014-10-20 0:06 GMT+08:00 Nikolay Sivov :
> > - str = SysAllocStringLen(NULL, 1023);
> > > - newstr = SysAllocStringLen(NULL, 1023);
> >
> > Where this length comes from?
> > Well, I tested String on windows xp, and found that 1023 was the limit,
> > when given a number bigger than that, the output kept the length of 1023.
>
> Can you add this too as a test? In some compact way if possible.

I tried again and I found that maybe you were right. The number of 1023 was
got like this: I wrote vbscript with a common text editor on xp, and save
it as xx.vbs, then double clicked it. I used this method and found that
MsgBox String(1024, 65) only printed "a...(the length is 1023)....a".
Tragedy is today I write MsgBox Len(String(1024, 65)), and it prints 1024!
It's the same with the other numbers greater than 1023!   So I  was cheated
yesterday. The true limit may be 32768 * 16384 * 1.25， but I am not sure. I
need to know the exact limit of the length of the string subtype, however
experiments show difference with the documents of microsoft which claims
the limit is about 2 billion.
> - 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;
> > > -
> > >    break;
> > > - }
> >
> > You only need first character, right? Then why do you need a full BSTR
> > pointer in VT_BSTR case? And assigning it
> > to 'str' you leak a previously allocated buffer.
>
> So how do I get the first character of (arg + 1)?  How about this: str[0]
> = * V_BSTR(arg + 1)
> In fact I don't quite understand how SysAllocStringLen work, but I see it
> is used in the former function, so I think maybe it is necessary.
>
> What I don't get is why do you allocate 'str' at all, it should be just
> 'WCHAR ch;' variable.

Yes, you are right.
> Why cast to (char)tmp?
> >
>
> I think the type of str[0] is WCHAR, and tmp is an integer, shouldn't we
> make a cast?
>
> WCHAR is 'unsigned short'. And it feels like it needs more tests for other
> variant types.

Yes, such as VT_DATE, but problem is it can't be tested. Until now, '#' is
still not supported in wine.
> > - else if(len == 0)
> > > -
> > >    newstr = '\0';
> >
> > Same way you're losing pointer to allocated buffer.

[PATCH] vbscript: Implemented String

[Nikolay Sivov]

On 10/23/2014 21:07, Shuai Meng wrote:
> Thanks for commenting.
>
> 2014-10-21 0:20 GMT+08:00 Nikolay Sivov <[e-mail] 
> >:
> On 10/20/2014 19:58, Shuai Meng wrote:
> > Thank you very much for commenting on this patch.
> >
> > 2014-10-20 0:06 GMT+08:00 Nikolay Sivov <[e-mail]
> > <mailto:[e-mail]>>:
> >
> >         +    str = SysAllocStringLen(NULL, 1023);
> >         +    newstr = SysAllocStringLen(NULL, 1023);
> >
> >     Where this length comes from?
> >     Well, I tested String on windows xp, and found that 1023 was
> >     the limit, when given a number bigger than that, the output
> >     kept the length of 1023.
> Can you add this too as a test? In some compact way if possible.
>
> I tried again and I found that maybe you were right. The number of 
> 1023 was got like this: I wrote vbscript with a common text editor on 
> xp, and save it as xx.vbs, then double clicked it. I used this method 
> and found that MsgBox String(1024, 65) only printed "a...(the length 
> is 1023)....a".
> Tragedy is today I write MsgBox Len(String(1024, 65)), and it prints 
> 1024! It's the same with the other numbers greater than 1023!   So I 
>  was cheated yesterday. The true limit may be 32768 * 16384 * 1.25， 
> but I am not sure. I need to know the exact limit of the length of the 
> string subtype, however experiments show difference with the documents 
> of microsoft which claims the limit is about 2 billion.

In that case we don't care about exact limit if any. Just allocate 
what's requested and fail with proper error code if allocation failed.