Conversation started Oct 16, 2018 at 0:14.
Oct 16, 2018 00:14
anyone has a 32-bit host handy?
@MathieuGuindon Does VB6 count?
not really, but I'd still be curious about the result
some guy sent me an email today, with some code and an alleged bug affecting 64-bit hosts
Eh? Was it in the unit tests?
no
StoringClass.cls
Option Explicit
Private mValue As Boolean

'Doesn't matter if this is commented out or not:
'
'Private Sub Class_Initialize()
'
'End Sub

Private Sub Class_Terminate()
'
End Sub

Public Property Get TheValue() As Boolean
    TheValue = mValue
End Property

Public Property Let TheValue(ByVal value As Boolean)
    mValue = value
End Property
then a stdmodule:
Option Explicit

Public Function CreateStoringClass(ByVal value As Boolean) As StoringClass
    Dim result As StoringClass
    Set result = New StoringClass
    result.TheValue = value
    Set CreateStoringClass = result
End Function


' Problem with Office 2013 64-Bit (incl. 2018-09, 15.0.5045.1000):
' This line of code works wrong:
' If Not Factory.CreateStoringClass(VALUE_TO_TEST).TheValue) Then

' Remark:
' The terminator in "StoringClass" causes this behaviour (even if it is empty).
' When it will be removed or commented out, it runs well!
there's a lot more junk and I've tweaked it a bit, but it does demonstrate the weirdness
output on 64-bit Excel is "This is wrong"
if I extract a local variable for the Create result, I get "This is correct"
Oct 16, 2018 00:19
^ my exact words
Ahah! I can remote in to work and test it.
curious if VB6 behaves the same way though
..well, 32-bit should be ok apparently
that makes one hell of a compelling argument for avoiding chained member calls on x64 VBA
I'll strip it to a bare-bones MCVE and send it to the Excel team
OK, logged in at work. Let's test this sucker in 32-bit.
Oct 16, 2018 00:22
Sub ShortTest()
    Const VALUE_TO_TEST As Boolean = True
    Debug.Print "Value to check: " & VALUE_TO_TEST

    Dim result As StoringClass
    Set result = CreateStoringClass(VALUE_TO_TEST)
    If result.TheValue <> VALUE_TO_TEST Then
        Debug.Print "This is wrong"
    Else
        Debug.Print "This is correct"
    End If
End Sub
^ works as expected
Sub ShortTest()
    Const VALUE_TO_TEST As Boolean = True
    Debug.Print "Value to check: " & VALUE_TO_TEST

    'Dim result As StoringClass
    'Set result = CreateStoringClass(VALUE_TO_TEST)
    If CreateStoringClass(VALUE_TO_TEST).TheValue <> VALUE_TO_TEST Then
        Debug.Print "This is wrong"
    Else
        Debug.Print "This is correct"
    End If
End Sub
^ doesn't
Stupid bi-directional clipboard...
ok even better
Works in 32-bit Excel.
ok now delete the Class_Terminate sub in the class
(I know it's empty)
on x64 it changes the behavior
i.e. removing it makes it work again
Same result on 32-bit - "This is correct"
Oct 16, 2018 00:27
ok good
MS broke VBA7... big time
and nobody noticed
that shows how often they use classes in VBA, I guess.
if you can prove it causes a buffer overrun, it'll be almost a certainty they will fix it
The Terminate thing is what I don't get, because it's acting like it doesn't get fully initialized.
Oct 16, 2018 00:29
@this I don't even know what that is
or any security holes, really.
If I can prove it causes a buffer overrun, I'll make them email it to themselves...
The point is that if you can show that it's a security-related hole, they will do everything in their powers to fix it.
including shipping a VBA8?
:D
Oct 16, 2018 00:30
lol. i wish
more likely, they'd ship a patch of 7.1 VBA? 7.2?
whatever it's now
Question re #4299: Given class Foo that implements IBar and given the procedure:
Sub FooBar(ByRef x As Foo)
    x.SomeIBarMethod
    Set x = New Foo
End Sub
Do you return the result 'Foo' can be declared as 'IBar'?
if SomeIBarMethod is declared on IBar, I don't see why not
My thought was that it could break the caller.
right... need to make sure all member calls are also on that interface
but then you're looking at a ByRef parameter...
Oct 16, 2018 00:36
like, if you sort that one out, might as well fix the false positives for byref-assigned variables reported as unassigned
#NotTrivial
Woah. You should put a Debug.Print "Terminating" into that buggy 64-bit code and step through it with the debugger.
not there yet lol, refactoring the thing to death...
but I imagine it's terminating too soon?
Yep - on the CreateStoringClass(VALUE_TO_TEST) line.
I knew to expect that, ....just not why
I should put it in a tight loop and see if it leaks memory.
Oct 16, 2018 00:40
@ThunderFrame will want to see this
I almost wonder if it's calling AddRef on the wrong instance.
Like if it screwed up on what pointer it was supposed to be using.
off-by-32-ish?
Something like that.
Actually, I'm not convinced that's a bug as much as undefined behavior.
No Class_Terminate handler, no local variable: OK
With Class_Terminate handler, no local variable: terminatingSurprise!
No Class_Terminate handler, with local variable: OK
With Class_Terminate handler, with local variable: OK
terminating
The spec says nothing of the lifespan of a transient object reference that I can find.
Oct 16, 2018 00:52
but the behavior being inconsistent between 32 and 64 bit hosts is a problem
There's no reason for ShortTest to bump a reference for it because it isn't holding one. That means it's free to be disposed.
except the refcount should account for the .Value member call, no?
Inconsistency is the hallmark of undefined behavior. You could make the argument that the 32-bit version is wrong.
@MathieuGuindon Does it scope down to the LoC?
What is the standard procedure for updating my local repository from the remote?
@jcrizk do you have local changes?
Oct 16, 2018 00:54
I mean, With holds a reference by spec. If?
I do, but the problem I was working on has been solved by some other solution
do you have rubberduck-vba/Rubberduck defined as upstream?
Probably not, unless this is done automatically
git remote -v
git remote add upstream github.com/rubberduck-vba/Rubberduck.git
git fetch upstream
git checkout next
git merge upstream/next
Oct 16, 2018 00:56
^ well, there ya go :)
Optionally solve merge conflicts.
Thanks!
@Comintern so RD needs an inspection for inlined member calls, on user objects (?), I guess. "Object may get terminated before the member call is executed, on 64-bit hosts"
I was just thinking that.
MemberAccessWithoutHeldReferenceInspection
What I want to know is if it's creating the default instance to call the Property Get.
OK, what sort of WTFery is this?
Option Explicit
Private mValue As Boolean

Private Sub Class_Initialize()
    Debug.Print "Initializing"
End Sub

Private Sub Class_Terminate()
    Debug.Print "Terminating"
End Sub

Public Property Get TheValue() As Boolean
    Debug.Print "Accessing TheValue"
    TheValue = mValue
End Property

Public Property Let TheValue(ByVal value As Boolean)
    Debug.Print "Setting TheValue"
    mValue = value
End Property
Output:
Value to check: True
Initializing
Setting TheValue
Accessing TheValue
Terminating
This is wrong
confirmed, With block works as intended
@Comintern wrong because the.. huh wait a sec
it's accessing the value before it nukes the instance???
Oct 16, 2018 01:03
^
that means the correct value is on the stack then
I don't follow. This seems expected?
up to This is wrong, yes
Just to confirm:
Option Explicit
Private mValue As Boolean
Private instance As Integer

Private Sub Class_Initialize()
    Static ctor As Integer
    ctor = ctor + 1
    instance = ctor
    Debug.Print "Initializing " & instance
End Sub

Private Sub Class_Terminate()
    Debug.Print "Terminating " & instance
End Sub

Public Property Get TheValue() As Boolean
    Debug.Print "Accessing TheValue " & instance
    TheValue = mValue
End Property

Public Property Let TheValue(ByVal value As Boolean)
    Debug.Print "Setting TheValue " & instance
Only 1 is created.
Value to check: True
Initializing 1
Setting TheValue 1
Accessing TheValue 1
Terminating 1
This is wrong
@MathieuGuindon but, This is wrong is outputted after the if statement....
so it has left the town...
Oct 16, 2018 01:07
i.e. the stack goes poof somewhere between the function returning the reference and the member call being made
or the wrong value is read from it
what if the type was Long instead of Boolean?
And on top of that, TheValue appears to be reading from uninitialized memory.
from a legal chunk of uninitialized memory, right? otherwise we'd long have gone down in flames..
can't believe we're just now witnessing undefined behavior in VBA
Value to check: True
Initialized
Setting the value
Getting the value
Getting the result
Terminated
This is correct
(this is on 32-bit host, though)
using this version of test:
Function ShortResult() As Boolean
    Debug.Print "Getting the result"
    ShortResult = True
End Function

Sub ShortTest1()
    Const VALUE_TO_TEST As Boolean = True
    Debug.Print "Value to check: " & VALUE_TO_TEST

    If CreateStoringClass(VALUE_TO_TEST).TheValue <> ShortResult Then
        Debug.Print "This is wrong"
    Else
        Debug.Print "This is correct"
    End If
End Sub
Same thing with a Long.
Value to check: 42
Initializing
Setting TheValue
Accessing TheValue
Terminating
This is wrong
yeah OP said 32-bit worked fine
@Comintern sucks we can't know the value without pulling a local variable though
Oct 16, 2018 01:12
IKR?
@MathieuGuindon that's why I used function above
Another idea -- wrap the expression in a function: If StoreTheResult(CreateStoringClass(VALUE_TO_TEST).TheValue <> ShortResult) Then
(actually we don't need the If really)
 
Conversation ended Oct 16, 2018 at 1:14.