[PATCH 2/5] activeds: Implement ADsOpenObject.

[Zebediah Figura]

On 12/16/19 3:21 AM, Dmitry Timoshkov wrote:
> Nikolay Sivov <bunglehead at gmail.com> wrote:
> 
>>>>>>> +                        hr =
>>> IADsOpenDSObject_OpenDSObject(adsopen,
>>>>> (BSTR)path, (BSTR)user, (BSTR)password, reserved, &disp);
>>>>>>> +                        if (hr == S_OK)
>>>>>> You cannot cast to a BSTR, it will corrupt the data structure.
>>>>>
>>>>> I depends wheather the callee uses SysStringLen() and friends, in this
>>>>> particular case it works just fine, there are many other places in the
>>> code
>>>>> when it's not necessary to create intermediate copies of the strings.
>>>>>
>>>>
>>>> It does not matter if it works in this case, arguments should use correct
>>>> types. If we have similar violations somewhere else, those need fixing as
>>>> well.
>>>
>>> That's a pretty incorrect assertion, there's nothing wrong in casting
>>> BSTR to LPWSTR and vice versa.
>>>
>>
>> What's incorrect about it? You can use BSTR as WCHAR * argument, obviously.
>> Not the other way around, even if it happens to work.
> 
> You are contradicting yourself: if it works that means that's correct
> behaviour. If you are going to further argue, please show the code that
> won't work for this particular implementation.
> 

I think it's just a matter of following correct and documented behaviour
on principle. You wouldn't intentionally use an object after calling
Release() just because it happens to work (relying on the fact that the
object is static, or that the heap memory just doesn't get overwritten).

Native might already have used something like SysStringLen() internally
and have corrupted data structures that simply aren't causing any
problems yet with this test. It also might not, but it costs very little
to just do the documented thing.