Visual D 0.50.0-beta1 released

[Bert]

On Sunday, 28 July 2019 at 11:45:21 UTC, Rainer Schuetze wrote:
>
>
> On 28/07/2019 06:59, Bert wrote:
>> On Wednesday, 24 July 2019 at 15:32:01 UTC, Rainer Schuetze 
>> wrote:
>>>
>>>
>>> On 24/07/2019 08:33, Bert wrote:
>>>> On Tuesday, 23 July 2019 at 20:57:17 UTC, Rainer Schuetze 
>>>> wrote
>>>>> As it turned out, the problem was (again?) a false entry in 
>>>>> the class
>>>>> name cache that was added when an invalid pointer 
>>>>> (including null) was
>>>>> evaluated (and its dynamic was tried to be determined). 
>>>>> Stupid bug, but
>>>>> that happens.
>>>>>
>>>>> You can try the fixed version of MagoNatCC.dll from 
>>>>> https://ci.appveyor.com/project/rainers/mago/build/artifacts
>>>>
>>>> That worked! I still find it very odd that it would exhibit 
>>>> it with
>>>> irrelevant lines of code removed. Was that changing the 
>>>> cached
>>>> version or something? Remember I basically have several Q's 
>>>> in my
>>>> program all virtually identical and some would work and 
>>>> others wouldn't.
>>>>
>>>> Could you post some of the code that you fixed(or I guess 
>>>> push it to master) when you get a chance... I'm just curious 
>>>> to what it looks like. While such bugs are very annoying and 
>>>> better coding methods should be used, I imagine that this 
>>>> type of scenario for interface resolution is probably well 
>>>> localized and this would probably be the last bug fix for 
>>>> it? If not maybe some logging could be implemented. 
>>>> Interfaces should always be resolvable so if the code fails 
>>>> it is a bug in Visual D and having it logged in some way 
>>>> might help future cases.
>>>
>>> The bugfix commit is 
>>> https://github.com/rainers/mago/commit/9801da089a4b4e9ffa8f1442f4b0aff5852d5fa9?w=1.
>>>
>>>
>>> The problem was that evaluating the dynamic type of any 
>>> uninitialized or null class/interface pointer could overwrite 
>>> a previously evaluated entry in the vtbl->class name cache 
>>> depending on what happens to be in an uninitialized local 
>>> variable.
>>>
>>> Hopefully the last bug in that area, but you never know. 
>>> Adding logging would help diagnosing bugs but would have to 
>>> be more global.
>> 
>> Thanks.
>> 
>> 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].


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?