Visual D 0.50.0-beta1 released

[Rainer Schuetze]

On 01/08/2019 05:59, Bert wrote:
> On Sunday, 28 July 2019 at 11:45:21 UTC, Rainer Schuetze wrote:
>>
>>
>> On 28/07/2019 06:59, Bert wrote:
>>> I can't imagine though why adding a simple field would screw the code
>>> up.
>>>
>>> e.g.,
>>>
>>> class X {} // works.
>>> class Y { int y; } // fails.
>>>
>>
>> This wouldn't have failed, int needs a class/interface member, e.g.
>>
>> class X
>> {
>>    Q q; // non-null
>>    I c; // null
>>    Q s; // non-null
>> }
>>
>>> I would think that such things would have no effect on the pointer
>>> value?
>>>
>>> Can you explain to me, for my own edification, why removing those
>>> non-essential lines would produce the effect?
>>>
>>
>> The bug basically was similar to
>>
>> // global cache
>> string[void*] mapVtblToClassName;
>>
>> void showX(X x)
>> {
>>    void* vtbl;
>>    string classNameq;
>>    bool rc = ReadProcessMemory(x.q, vtbl); // reads vtbl-ptr
>>    if (rc)
>>    {
>>       if (vtbl in mapVtblToClassName)
>>       {
>>          classNameq = mapVtblToClassName[vtbl];
>>          goto Lnext;
>>       }
>>       classNameq = getClassNameFromVtbl(vtbl); // Q
>>    }
>>    mapVtblToClassName[vtbl] = classNameq;
>>
>> Lnext:
>>    string classNamec;
>>    rc = ReadProcessMemory(x.c, vtbl); // fails, vtbl unchanged
>>    if (rc)
>>    {
>>       // same as above...
>>    }
>>    mapVtblToClassName[vtbl] = classNamec; // overwrites entry for Q
>> }
>>
>> Next time a Q is evaluated, the determined class name will be empty.
> 
> 
> Thanks. Ok, I see how the caching can be overwritten but I still don't
> see how changing a field in the class, which won't even modify the
> vtable, will effect the cache. I'd expect some sort of determinism in
> this, in that if I compile the some code and everything works and then
> simply add a field to a class and recompile, little should change.
> 
> E.g., if the cache is being overwritten in the first case then adding
> the field shouldn't change it from being overwritten in the second case.
> 
> Oh, nevermind,
> 
> ReadProcessMemory(x.q, vtbl);
> 
> you are iterating over the entries in x to get their vtable! So changing
> a field could cause a shift in the execution order and hence initiate
> the overwrite. (some of the code is missing above but I see how it could
> lead to these problems).
> 
> While I still don't fully understand why the code is doing what it
> does(e.g., q and c? I assume these are suppose to be members). Seems
> there doesn't need to be a goto and 2nd branch in theory but I guess
> it's incomplete code so I'll assume it all is suppose to work[Or you are
> just showing a second iteration].

Yes, this was pseudo code showing the effect showing two calls to the
same function. I used "goto" where a function would use "return".

> 
> 
> Essentially just one part of the code is buggy because classNameq is
> empty and it can blindly overwrite the vtbl value *if* any future
> iterations occur where the if(rc) check fails(e.g., an added field).
> 
>>    void* vtbl;
>>    string classNameq;
>>    bool rc = ReadProcessMemory(x.q, vtbl); // reads vtbl-ptr
>>    if (rc)
>>    {
>>       if (vtbl in mapVtblToClassName)
>>       {
>>          classNameq = mapVtblToClassName[vtbl];
>>          goto Lnext;
>>       }
>>       classNameq = getClassNameFromVtbl(vtbl); // Q
>>    }
>>    mapVtblToClassName[vtbl] = classNameq;
> 
> 
> which then something like
> 
> if (classNameq) mapVtblToClassName[vtbl] = classNameq;
> 
> would fix the problem?
> 
> 
> 

The empty classname is also cached for invalid vtbl pointers (e.g. when
reading uninitialized data). The fix is to move the insertion into the
map into the "if(rc)" branch.