Dangling pointers when registering `QMetaObject`

[arnetheduck]

https://github.com/filcuc/nimqml/blob/105f5abe38570147cf95191416a24dfbb3f1055c/src/nimqml/private/qmetaobject.nim#L23

parameters is a local variable that may get deallocated, yet the code stores pointers to it that outlive its lifetime - the safer option is to use values from the seq inside QMetaObject since the latter is a ref and the former don't change.

[filcuc]

For sure the code is not the cleanest but i understood that the "seq" where heap allocated in practice. The idea was to "move" the heap allocated pointer of the seq at line 24 https://github.com/filcuc/nimqml/blob/105f5abe38570147cf95191416a24dfbb3f1055c/src/nimqml/private/qmetaobject.nim#L24 the outer dosParameters seq
https://github.com/filcuc/nimqml/blob/105f5abe38570147cf95191416a24dfbb3f1055c/src/nimqml/private/qmetaobject.nim#L14 for keeping it's data alive and keeping the "pointers" stable

[arnetheduck]

The idea was to "move" the heap allocated pointer of the seq at line 24

moves don't reliably happen in nim 1.x / gc:refc (it copies instead) and are in general a "best-effort" feature (ie subtle refactoring will break it) - the reason it still works sort of is that garbage collection gets deferred if the lists are small enough

[arnetheduck]

it's easy to see this in the generated code: here's what 1.6 generates:

                dosParameters = (tySequence__1CqTOQNamVPtZBeU0cHxCg*)incrSeqV3(
                    (TGenericSeq*)(dosParameters), (&NTIseqLseqLdosparameterdefinitionTT__1CqTOQNamVPtZBeU0cHxCg_));
                T42_ = dosParameters->Sup.len++;
                genericSeqAssign((&dosParameters->data[T42_]),
                                 parameters_2,
                                 (&NTIseqLdosparameterdefinitionT__i8Zb4CqOisj2uXxNWH9bMnA_));

you can see how genericSeqAssign copies the parameters seq (that's what assign does - it doesn't move)

[filcuc]

Ok I will try to fix this asap

Il giorno lun 27 feb 2023 alle 12:56 Jacek Sieka @.***> ha scritto:

it's easy to see this in the generated code: here's what 1.6 generates:

            dosParameters = (tySequence__1CqTOQNamVPtZBeU0cHxCg*)incrSeqV3(
                (TGenericSeq*)(dosParameters), (&NTIseqLseqLdosparameterdefinitionTT__1CqTOQNamVPtZBeU0cHxCg_));
            T42_ = dosParameters->Sup.len++;
            genericSeqAssign((&dosParameters->data[T42_]),
                             parameters_2,
                             (&NTIseqLdosparameterdefinitionT__i8Zb4CqOisj2uXxNWH9bMnA_));

you can see how genericSeqAssign copies the parameters seq (that's what assign does - it doesn't move)

— Reply to this email directly, view it on GitHub https://github.com/filcuc/nimqml/issues/48#issuecomment-1446197118, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABWTXI7ERFTHQVXDXUZFNY3WZSI65ANCNFSM6AAAAAAVIKPQSE . You are receiving this because you commented.Message ID: @.***>

-- Filippo Cucchetto

[filcuc]

@arnetheduck i've opened https://github.com/filcuc/nimqml/pull/49 mind to give your opinion. Basically the idea is to just create the parameters array before in order to make in stable (instead of creating it inside the for loop)