[L2Ork-dev] Segfault using paste from clipboard on Linux
Jonathan Wilkes
jon.w.wilkes at gmail.com
Thu Jun 18 22:40:04 EDT 2020
On Thu, Jun 18, 2020 at 9:48 PM Ivica Bukvic <ico at vt.edu> wrote:
>
> Can you explain what that for loop does as if I am 5 yo? I do not see any checks here where to stop and eventually you will hit the last object whose g_next is NULL. Does it check in the middle condition if g2 is not NULL after it was assigned a new value?
It sets "g" to the last non-null node in the glist. I.e., this would
be the last object that was created on the given canvas.
canvas_f is a canvas method, so its "x" (i.e., a poor c programmer's
version of "this") is the canvas. But the point of "f"
(format?) here is to change the width of a particular object. So this
function must find the last object that was
created on the canvas in order to set its new width and redisplay it.
This happens for each line read in from a Pd file
that has a ", f <number>;" suffix, plus the special case of subpatches
which have their own "#X f <number>;" line.
Because compsci is an art and not a science, professors teach data
structures like linked lists. They are clunky to use,
but at least they're slow to iterate. (And in the case of Pd, it's the
iteration that's in the hot path,
not the insertion/deletion. As Bjarne Stroustrup put it, "you're
optimizing for cache misses." :)
So instead of foo.length - 1, you get a handmade waterwheel with
rustic design handed down for generations.
But if valgrind is telling you a plank of wood is missing, it's
possible it was missing before you got to canvas_f.
-Jonathan
>
> Best,
>
> Ico
>
> --
> Ivica Ico Bukvic, D.M.A.
> Director, Creativity + Innovation
> Institute for Creativity, Arts, and Technology
>
> Virginia Tech
> Creative Technologies in Music
> School of Performing Arts – 0141
> Blacksburg, VA 24061
> (540) 231-6139
> ico at vt.edu
>
> www.icat.vt.edu
> www.performingarts.vt.edu
> l2ork.icat.vt.edu
> ico.bukvic.net
>
> On Thu, Jun 18, 2020, 21:44 Jonathan Wilkes <jon.w.wilkes at gmail.com> wrote:
>>
>> Woah-- wait a sec:
>>
>> --> for (g = x->gl_list; g2 = g->g_next; g = g2)
>> ;
>>
>> As weird as it is, that algorithm is sound. And the bug with ", f 12;"
>> was in the condition
>> before this one.
>>
>> If there is an invalid read by simply walking the glist, then the
>> glist has been corrupted.
>>
>> So I would check the code for pasting patch snippets to see if there
>> is anything that
>> is corrupting the glist.
>>
>> E.g., neglecting to set g->g_next for the final node. Stuff like that.
>>
>> -Jonathan
>>
>> On Thu, Jun 18, 2020 at 9:03 PM Jonathan Wilkes <jon.w.wilkes at gmail.com> wrote:
>> >
>> > On Thu, Jun 18, 2020 at 8:55 PM Ivica Ico Bukvic <ico at vt.edu> wrote:
>> > >
>> > > I am not even sure if we need to check for argc... That said, the
>> > > warning for some reason is not being triggered.
>> >
>> > Please run purr-data/scripts/regression_test.pd to make sure this
>> > isn't breaking anything.
>> >
>> > >
>> > > Lastly, please note that creating a patch in purr-data and saving it
>> > > still results in the legacy <command>, f <num>; format for describing
>> > > the custom width of an object.
>> >
>> > Ooh, we need to change that.
>> >
>> > We should open both formats and only save the sane one.
>> >
>> > -Jonathan
>> >
>> > >
>> > > Best,
>> > >
>> > > Ico
>> > >
>> > > On 6/18/2020 8:53 PM, Ivica Ico Bukvic wrote:
>> > > > How about simply changing the following inside canvas_f:
>> > > >
>> > > > if (pd_class(last_typedmess_pd) == canvas_class &&
>> > > > (t_canvas *)last_typedmess_pd == x &&
>> > > > last_typedmess == gensym("restore"))
>> > > >
>> > > > to:
>> > > >
>> > > > if (s == gensym("f") && argc >= 1)
>> > > >
>> > > > Seems to have fixed the segfault on Linux and Windows.
>> > > >
>> > > > Best,
>> > > >
>> > > > Ico
>> > > >
>> > > > On 6/18/2020 4:53 PM, Jonathan Wilkes wrote:
>> > > >> On Thu, Jun 18, 2020 at 4:29 PM Ivica Ico Bukvic <ico at vt.edu> wrote:
>> > > >>> It's by far the most SNAFU'd implementation of a for loop I've ever
>> > > >>> seen. So much so, I am not even sure what it exactly does:
>> > > >>>
>> > > >>> --> for (g = x->gl_list; g2 = g->g_next; g = g2)
>> > > >>> ;
>> > > >> This is all about that inane ", f $value" syntax that Miller wrote,
>> > > >> didn't
>> > > >> test, and then got changed in a later Pd Vanilla version.
>> > > >>
>> > > >> This bug is probably from Purr Data attempted to support this
>> > > >> deprecated ", f 12;"
>> > > >> syntax plus the fixed behavior that just uses "#X f 12;" on the next
>> > > >> line.
>> > > >>
>> > > >> I'd like to continue supporting both syntaxes, otherwise uses of the
>> > > >> old syntax
>> > > >> will hit the bug and be sad.
>> > > >>
>> > > >> See the note in line above `t_symbol *last_typedmess;` to understand
>> > > >> what's going
>> > > >> on.
>> > > >>
>> > > >> I'll drill down on this later and see what's up.
>> > > >>
>> > > >> And just to be clear-- I only attempted to do damage control around that
>> > > >> looper, I didn't write it. (Yet another reason why we should just be
>> > > >> using arrays,
>> > > >> but that's for another time.)
>> > > >>
>> > > >> -Jonathan
>> > > >>
>> > > >>> I am assuming it is looking for the last element in the gl_list. If so,
>> > > >>> I have no idea how this has worked so far, since I do not see any kind
>> > > >>> of a check whether g->g_next is not NULL... This is also the only place
>> > > >>> in the entire function (canvas_f) that uses t_gobj *g2.
>> > > >>>
>> > > >>> Running uner that assumption, I refactored it as:
>> > > >>>
>> > > >>> g = x->gl_list;
>> > > >>> if (g) // probably unnecessary but hey let's be pedantic
>> > > >>> while (g->g_next)
>> > > >>> g = g->g_next;
>> > > >>>
>> > > >>> Once this is implemented, the segfault now happens on line 2282
>> > > >>> (immediately below) on the first pd_checkobject(&g->g_pd) which also
>> > > >>> segfaults with an invalid read of size 8...
>> > > >>>
>> > > >>>
>> > > >>> On 6/18/2020 4:13 PM, Jonathan Wilkes wrote:
>> > > >>>> On Thu, Jun 18, 2020 at 3:32 PM Ivica Bukvic <ico at vt.edu> wrote:
>> > > >>>>> Valgrind is a bit more descriptive. It does not happen every time
>> > > >>>>> but it does happen nonetheless. Looks like Windows is a lot less
>> > > >>>>> forgiving. I was pasting the content of a simple patch multiple
>> > > >>>>> times (close the window, new window, paste again):
>> > > >>>>>
>> > > >>>>> #N canvas 487 261 450 300 10;
>> > > >>>>> #X floatatom 145 63 5 0 0 0 - - -, f 5;
>> > > >>>>> #X obj 241 123 print;
>> > > >>>>> #X floatatom 126 134 5 0 0 0 - - -, f 5;
>> > > >>>>> #X connect 0 0 1 0;
>> > > >>>>>
>> > > >>>>> ==12982== Invalid read of size 8
>> > > >>>>> ==12982== at 0x436EF3: canvas_f (g_canvas.c:2273)
>> > > >>>> Something's off for me because 2273 isn't inside canvas_f().
>> > > >>>>
>> > > >>>> Can you tell me what's at that line number in whatever is your
>> > > >>>> current branch?
>> > > >>>>
>> > > >>>> -Jonathan
>> > > >>>>
>> > > >>>>> ==12982== by 0x49B977: pd_typedmess (m_class.c:779)
>> > > >>>>> ==12982== by 0x49B7DD: pd_typedmess (m_class.c:883)
>> > > >>>>> ==12982== by 0x4A4D80: binbuf_eval (m_binbuf.c:937)
>> > > >>>>> ==12982== by 0x4AC56B: socketreceiver_read (s_inter.c:615)
>> > > >>>>> ==12982== by 0x4AB554: sys_domicrosleep.constprop.3
>> > > >>>>> (s_inter.c:226)
>> > > >>>>> ==12982== by 0x4AE35A: sys_pollgui (s_inter.c:1155)
>> > > >>>>> ==12982== by 0x4A84B9: m_pollingscheduler (m_sched.c:542)
>> > > >>>>> ==12982== by 0x4A84B9: m_mainloop (m_sched.c:622)
>> > > >>>>> ==12982== by 0x4AAEF9: sys_main (s_main.c:440)
>> > > >>>>> ==12982== by 0x5ED882F: (below main) (libc-start.c:291)
>> > > >>>>> ==12982== Address 0x8 is not stack'd, malloc'd or (recently) free'd
>> > > >>>>> ==12982==
>> > > >>>>> ==12982==
>> > > >>>>> ==12982== Process terminating with default action of signal 11
>> > > >>>>> (SIGSEGV)
>> > > >>>>> ==12982== Access not within mapped region at address 0x8
>> > > >>>>> ==12982== at 0x436EF3: canvas_f (g_canvas.c:2273)
>> > > >>>>> ==12982== by 0x49B977: pd_typedmess (m_class.c:779)
>> > > >>>>> ==12982== by 0x49B7DD: pd_typedmess (m_class.c:883)
>> > > >>>>> ==12982== by 0x4A4D80: binbuf_eval (m_binbuf.c:937)
>> > > >>>>> ==12982== by 0x4AC56B: socketreceiver_read (s_inter.c:615)
>> > > >>>>> ==12982== by 0x4AB554: sys_domicrosleep.constprop.3
>> > > >>>>> (s_inter.c:226)
>> > > >>>>> ==12982== by 0x4AE35A: sys_pollgui (s_inter.c:1155)
>> > > >>>>> ==12982== by 0x4A84B9: m_pollingscheduler (m_sched.c:542)
>> > > >>>>> ==12982== by 0x4A84B9: m_mainloop (m_sched.c:622)
>> > > >>>>> ==12982== by 0x4AAEF9: sys_main (s_main.c:440)
>> > > >>>>> ==12982== by 0x5ED882F: (below main) (libc-start.c:291)
>> > > >>>>> ==12982== If you believe this happened as a result of a stack
>> > > >>>>> ==12982== overflow in your program's main thread (unlikely but
>> > > >>>>> ==12982== possible), you can try to increase the size of the
>> > > >>>>> ==12982== main thread stack using the --main-stacksize= flag.
>> > > >>>>> ==12982== The main thread stack size used in this run was 8388608.
>> > > >>>>> ==12982==
>> > > >>>>> ==12982== HEAP SUMMARY:
>> > > >>>>> ==12982== in use at exit: 355,586 bytes in 3,885 blocks
>> > > >>>>> ==12982== total heap usage: 7,748 allocs, 3,863 frees, 2,870,428
>> > > >>>>> bytes allocated
>> > > >>>>> ==12982==
>> > > >>>>> ==12982== LEAK SUMMARY:
>> > > >>>>> ==12982== definitely lost: 416 bytes in 9 blocks
>> > > >>>>> ==12982== indirectly lost: 85 bytes in 10 blocks
>> > > >>>>> ==12982== possibly lost: 43,438 bytes in 1,323 blocks
>> > > >>>>> ==12982== still reachable: 311,647 bytes in 2,543 blocks
>> > > >>>>> ==12982== suppressed: 0 bytes in 0 blocks
>> > > >>>>> ==12982== Rerun with --leak-check=full to see details of leaked
>> > > >>>>> memory
>> > > >>>>> ==12982==
>> > > >>>>> ==12982== For counts of detected and suppressed errors, rerun
>> > > >>>>> with: -v
>> > > >>>>> ==12982== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0
>> > > >>>>> from 0)
>> > > >>>>>
>> > > >>>>> --
>> > > >>>>> Ivica Ico Bukvic, D.M.A.
>> > > >>>>> Director, Creativity + Innovation
>> > > >>>>> Institute for Creativity, Arts, and Technology
>> > > >>>>>
>> > > >>>>> Virginia Tech
>> > > >>>>> Creative Technologies in Music
>> > > >>>>> School of Performing Arts – 0141
>> > > >>>>> Blacksburg, VA 24061
>> > > >>>>> (540) 231-6139
>> > > >>>>> ico at vt.edu
>> > > >>>>>
>> > > >>>>> ci.icat.vt.edu
>> > > >>>>> www.icat.vt.edu
>> > > >>>>> www.performingarts.vt.edu
>> > > >>>>> l2ork.icat.vt.edu
>> > > >>>>> ico.bukvic.net
>> > > >>> --
>> > > >>> Ivica Ico Bukvic, D.M.A.
>> > > >>> Director, Creativity + Innovation
>> > > >>> Institute for Creativity, Arts, and Technology
>> > > >>>
>> > > >>> Virginia Tech
>> > > >>> Creative Technologies in Music
>> > > >>> School of Performing Arts – 0141
>> > > >>> Blacksburg, VA 24061
>> > > >>> (540) 231-6139
>> > > >>> ico at vt.edu
>> > > >>>
>> > > >>> www.icat.vt.edu
>> > > >>> www.performingarts.vt.edu
>> > > >>> l2ork.icat.vt.edu
>> > > >>> ico.bukvic.net
>> > > >>>
>> > > >> .
>> > > >
>> > > --
>> > > Ivica Ico Bukvic, D.M.A.
>> > > Director, Creativity + Innovation
>> > > Institute for Creativity, Arts, and Technology
>> > >
>> > > Virginia Tech
>> > > Creative Technologies in Music
>> > > School of Performing Arts – 0141
>> > > Blacksburg, VA 24061
>> > > (540) 231-6139
>> > > ico at vt.edu
>> > >
>> > > www.icat.vt.edu
>> > > www.performingarts.vt.edu
>> > > l2ork.icat.vt.edu
>> > > ico.bukvic.net
>> > >
More information about the L2Ork-dev
mailing list