Buffer overflow when LTO is in place

[falkTX]

For some reason surge-xt is crashing when LTO is enabled, but runs fine otherwise.

This is related to Cardinal yes, where I enable LTO for final release builds. On a big project like Cardinal the LTO makes a lot of sense, since there is a lot of code the compiler+linker can optimize for.

I do LTO builds with -Werror=odr -Werror=lto-type-mismatch so the linking fails if any type/class/struct has a mismatch between compiled objects. For surge-xt I setup "HysteresisProcessing Patch SolverType Tunings Wavetable ghc clouds plaits stmlib" as the names to override with a prefix so it prevents symbol collision with other existing modules. Final LTO build results in no conflicts. But we still get a crash when adding surge modules..

Do you happen to know why?

[baconpaul]

No clue. I’ve never used lto

[falkTX]

When having an LTO with debug symbols, we can see the following trace in gdb:

[6.763 info Rack/src/app/Browser.cpp:201 createPreview] Creating module widget Surge XT Delay
[9.292 info Rack/src/app/Browser.cpp:89 chooseModel] Creating module Surge XT Sine VCO
[9.292 info surgext/src/XTModule.h:68 showBuildInfo] [SurgeXTRack] Instance: Module=VCO<Sine> BuildInfo=os:linux pluggit:Cardinal surgegit:Cardinal buildtime=Dec  4 2022 17:47:45
*** buffer overflow detected ***: terminated

Thread 1 "carla-bridge-na" received signal SIGABRT, Aborted.
__pthread_kill_implementation (no_tid=0, signo=6, threadid=140737340186304) at ./nptl/pthread_kill.c:44
44      ./nptl/pthread_kill.c: No such file or directory.
(gdb) bt
#0  __pthread_kill_implementation (no_tid=0, signo=6, threadid=140737340186304) at ./nptl/pthread_kill.c:44
#1  __pthread_kill_internal (signo=6, threadid=140737340186304) at ./nptl/pthread_kill.c:78
#2  __GI___pthread_kill (threadid=140737340186304, signo=signo@entry=6) at ./nptl/pthread_kill.c:89
#3  0x00007ffff7580476 in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26
#4  0x00007ffff75667f3 in __GI_abort () at ./stdlib/abort.c:79
#5  0x00007ffff75c76f6 in __libc_message (action=action@entry=do_abort, fmt=fmt@entry=0x7ffff7719943 "*** %s ***: terminated\n") at ../sysdeps/posix/libc_fatal.c:155
#6  0x00007ffff767476a in __GI___fortify_fail (msg=msg@entry=0x7ffff77198e9 "buffer overflow detected") at ./debug/fortify_fail.c:26
#7  0x00007ffff76730c6 in __GI___chk_fail () at ./debug/chk_fail.c:28
#8  0x00007ffff7672d05 in ___snprintf_chk (s=<optimized out>, maxlen=<optimized out>, flag=<optimized out>, slen=<optimized out>, format=<optimized out>) at ./debug/snprintf_chk.c:29
#9  0x00007fffec0be358 in snprintf (__fmt=<optimized out>, __n=<optimized out>, __s=<optimized out>, __s=<optimized out>, __n=<optimized out>, __fmt=<optimized out>) at /usr/include/x86_64-linux-gnu/bits/stdio2.h:71
#10 get_prefix (scene=0, ctrlgroup_entry=0, ctrlgroup=cg_GLOBAL, txt=0x7fffffff5590 "n") at /home/falktx/Source/DISTRHO/Cardinal.lto/plugins/surgext/surge/src/common/Parameter.cpp:78
#11 Parameter::assign (this=0x7fff9c396ff8, idp=..., pid=<optimized out>, name=0x7fffed9dc670 "volume_FX1", dispname=<optimized out>, ctrltype=22, ui_identifier="global.fx1_return", posx=763, posy=141, scene=0, 
    ctrlgroup=cg_GLOBAL, ctrlgroup_entry=0, modulateable=true, ctrlstyle=1, defaultDeactivation=true) at /home/falktx/Source/DISTRHO/Cardinal.lto/plugins/surgext/surge/src/common/Parameter.cpp:190
#12 0x00007fffec47e95c in Parameter::assign(std::shared_ptr<ParameterIDCounter::ParameterIDPromise>, int, char const*, char const*, int, Surge::Skin::Connector const&, int, ControlGroup, int, bool, int, bool) [clone .constprop.0] (this=0x7fff9c396ff8, idp=std::shared_ptr<ParameterIDCounter::ParameterIDPromise> (use count 3, weak count 0) = {...}, name=0x7fffed9dc670 "volume_FX1", dispname=0x7fffed9dc65f "Send FX 1 Return", 
    ctrltype=22, c=..., scene=0, ctrlgroup=cg_GLOBAL, ctrlgroup_entry=0, modulateable=true, ctrlstyle=1, defaultDeactivation=true, pid=0)
    at /home/falktx/Source/DISTRHO/Cardinal.lto/plugins/surgext/surge/src/common/Parameter.cpp:162
#13 0x00007fffec0dff94 in SurgePatch::SurgePatch (storage=0x555558f4d4d0, this=<optimized out>) at /home/falktx/Source/DISTRHO/Cardinal.lto/plugins/surgext/surge/src/common/SurgePatch.cpp:41
#14 SurgeStorage::SurgeStorage (this=<optimized out>, suppliedDataPath=..., this=<optimized out>, suppliedDataPath=...) at /home/falktx/Source/DISTRHO/Cardinal.lto/plugins/surgext/surge/src/common/SurgeStorage.cpp:82
#15 0x00007fffec4a7b64 in std::make_unique<SurgeStorage, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&> () at /usr/include/c++/11/bits/unique_ptr.h:962
#16 sst::surgext_rack::modules::XTModule::setupSurgeCommon(int, bool) [clone .constprop.0] (this=this@entry=0x555558f03990, loadWavetables=loadWavetables@entry=false, NUM_PARAMS=<optimized out>)
    at ../../plugins/surgext/src/XTModule.h:98
#17 0x00007fffebd30142 in sst::surgext_rack::vco::VCO<1>::VCO (this=<optimized out>, this=<optimized out>) at ../../plugins/surgext/src/VCO.h:144
#18 0x00007fffebcff3b0 in rack::CardinalPluginModel<sst::surgext_rack::vco::VCO<1>, sst::surgext_rack::vco::ui::VCOWidget<1> >::createModule (this=0x55555650c260) at ../../plugins/../include/helpers.hpp:63
#19 0x00007fffec01971a in rack::app::browser::chooseModel (model=0x55555650c260) at ../Rack/src/app/Browser.cpp:90
#20 0x00007fffec019bb1 in rack::app::browser::ModelBox::onButton (this=0x5555574f85c0, e=...) at ../Rack/src/app/Browser.cpp:259
#21 0x00007fffeab6c4f4 in rack::widget::Widget::recursePositionEvent<void (rack::widget::Widget::*)(rack::widget::Widget::ButtonEvent const&), rack::widget::Widget::ButtonEvent> (e=..., f=&virtual table offset 96, 
    this=0x55555748da30) at ../../plugins/../src/Rack/include/widget/Widget.hpp:197
#22 rack::widget::Widget::onButton (this=0x55555748da30, e=...) at ../../plugins/../src/Rack/include/widget/Widget.hpp:234
#23 0x00007fffeab6c4f4 in rack::widget::Widget::recursePositionEvent<void (rack::widget::Widget::*)(rack::widget::Widget::ButtonEvent const&), rack::widget::Widget::ButtonEvent> (e=..., f=&virtual table offset 96, 
    this=0x555557507d80) at ../../plugins/../src/Rack/include/widget/Widget.hpp:197
#24 rack::widget::Widget::onButton (this=0x555557507d80, e=...) at ../../plugins/../src/Rack/include/widget/Widget.hpp:234
#25 0x00007fffeab6c4f4 in rack::widget::Widget::recursePositionEvent<void (rack::widget::Widget::*)(rack::widget::Widget::ButtonEvent const&), rack::widget::Widget::ButtonEvent> (e=..., f=&virtual table offset 96, 
    this=0x555556bdb150) at ../../plugins/../src/Rack/include/widget/Widget.hpp:197
#26 rack::widget::Widget::onButton (this=0x555556bdb150, e=...) at ../../plugins/../src/Rack/include/widget/Widget.hpp:234
#27 0x00007fffebfdb767 in rack::widget::Widget::recursePositionEvent<void (rack::widget::Widget::*)(rack::widget::Widget::ButtonEvent const&), rack::widget::Widget::ButtonEvent> (e=..., f=&virtual table offset 96, 
    this=0x5555574fd5a0) at ../../plugins/../src/Rack/include/widget/Widget.hpp:197
#28 rack::widget::Widget::onButton (e=..., this=<optimized out>, this=<optimized out>, e=...) at ../Rack/include/widget/Widget.hpp:234
#29 rack::ui::ScrollWidget::onButton (this=0x5555574fd5a0, e=...) at ../Rack/src/ui/ScrollWidget.cpp:130
#30 0x00007fffebfda028 in rack::widget::Widget::recursePositionEvent<void (rack::widget::Widget::*)(rack::widget::Widget::ButtonEvent const&), rack::widget::Widget::ButtonEvent> (e=..., f=&virtual table offset 96, 
    this=0x7fffd00678e0) at ../../plugins/../src/Rack/include/widget/Widget.hpp:197
#31 rack::widget::Widget::onButton (e=..., this=<optimized out>, this=<optimized out>, e=...) at ../Rack/include/widget/Widget.hpp:234
#32 rack::app::browser::Browser::onButton (this=0x7fffd00678e0, e=...) at ../Rack/src/app/Browser.cpp:781
#33 0x00007fffebfdb368 in rack::widget::Widget::recursePositionEvent<void (rack::widget::Widget::*)(rack::widget::Widget::ButtonEvent const&), rack::widget::Widget::ButtonEvent> (e=..., f=&virtual table offset 96, 
    this=0x555557516100) at ../../plugins/../src/Rack/include/widget/Widget.hpp:197
#34 rack::widget::Widget::onButton (e=..., this=0x555557516100) at ../../plugins/../src/Rack/include/widget/Widget.hpp:234
#35 rack::widget::OpaqueWidget::onButton (e=..., this=0x555557516100) at ../../plugins/../src/Rack/include/widget/OpaqueWidget.hpp:21
#36 rack::ui::MenuOverlay::onButton (this=0x555557516100, e=...) at ../Rack/src/ui/MenuOverlay.cpp:34
--Type <RET> for more, q to quit, c to continue without paging--
#37 0x00007fffeab6cd48 in rack::widget::Widget::recursePositionEvent<void (rack::widget::Widget::*)(rack::widget::Widget::ButtonEvent const&), rack::widget::Widget::ButtonEvent> (e=..., f=&virtual table offset 96, 
    this=0x555557513da0) at ../../plugins/../src/Rack/include/widget/Widget.hpp:197
#38 rack::widget::Widget::onButton (e=..., this=0x555557513da0) at ../../plugins/../src/Rack/include/widget/Widget.hpp:234
#39 rack::widget::OpaqueWidget::onButton (this=0x555557513da0, e=...) at ../../plugins/../src/Rack/include/widget/OpaqueWidget.hpp:21
#40 0x00007fffeab59fe5 in rack::widget::EventState::handleButton (mods=0, action=1, button=0, pos=..., this=0x55555748def0) at ../Rack/src/widget/event.cpp:134
#41 CardinalDISTRHO::CardinalUI::onMouse (this=<optimized out>, ev=...) at /home/falktx/Source/DISTRHO/Cardinal.lto/src/Cardinal/CardinalUI.cpp:825
#42 0x00007fffeab5efd8 in CardinalDGL::Window::PrivateData::onPuglMouse (ev=..., this=0x5555576f04e0) at ../../dpf/dgl/src/WindowPrivateData.cpp:720
#43 CardinalDGL::Window::PrivateData::onPuglMouse (ev=..., this=0x5555576f04e0) at ../../dpf/dgl/src/WindowPrivateData.cpp:708
#44 CardinalDGL::Window::PrivateData::puglEventCallback (view=<optimized out>, event=<optimized out>) at ../../dpf/dgl/src/WindowPrivateData.cpp:992
#45 0x00007fffeab8f57c in CardinalDGL::puglDispatchEvent (view=view@entry=0x5555576108b0, event=0x7fffffffd4b0) at ../../dpf/dgl/src/pugl-upstream/src/internal.c:186
#46 0x00007fffeab8fe50 in CardinalDGL::dispatchX11Events (world=0x55555764bef0) at ../../dpf/dgl/src/pugl-upstream/src/x11.c:1467
#47 0x00007fffec58d673 in CardinalDGL::puglUpdate(CardinalDGL::PuglWorldImpl*, double) [clone .isra.0] (world=0x55555764bef0, timeout=timeout@entry=0) at ../../dpf/dgl/src/pugl-upstream/src/x11.c:1494
#48 0x00007fffec5920a5 in CardinalDGL::Application::PrivateData::idle (timeoutInMs=0, this=0x55555764afd0) at ../../dpf/dgl/src/ApplicationPrivateData.cpp:122
#49 CardinalDGL::Application::idle() [clone .isra.0] (this=<optimized out>, this=<optimized out>) at ../../dpf/dgl/src/Application.cpp:46
#50 0x00007fffeab3d6f6 in CardinalDISTRHO::UIExporter::plugin_idle (this=0x55555764b298) at ../../dpf/distrho/src/DistrhoUIInternal.hpp:247
#51 CardinalDISTRHO::UIVst3::onTimer (this=0x55555764b260) at ../../dpf/distrho/src/DistrhoUIVST3.cpp:688
#52 CardinalDISTRHO::dpf_timer_handler::on_timer (self=<optimized out>) at ../../dpf/distrho/src/DistrhoUIVST3.cpp:1160
#53 0x0000555555647bb6 in ?? ()
#54 0x0000555555647063 in ?? ()
#55 0x0000555555646800 in ?? ()
#56 0x000055555564a6c9 in ?? ()
#57 0x000055555549a93e in ?? ()
#58 0x0000555555486f87 in ?? ()
#59 0x00007ffff7567d90 in __libc_start_call_main (main=main@entry=0x555555486780, argc=argc@entry=5, argv=argv@entry=0x7fffffffdc58) at ../sysdeps/nptl/libc_start_call_main.h:58
#60 0x00007ffff7567e40 in __libc_start_main_impl (main=0x555555486780, argc=5, argv=0x7fffffffdc58, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, stack_end=0x7fffffffdc48)
    at ../csu/libc-start.c:392
#61 0x0000555555499f4a in ?? ()

[baconpaul]

Right

So parameter assign connects to the static data members of the skin like I mentioned - that surge skin connector is made at static construction time

I wonder if your lto is optimizing out some dll statics

[baconpaul]

https://github.com/surge-synthesizer/surge/blob/3241a773ecbe7271955f70ba179bb9e8a3c4b2a6/src/common/SkinModel.cpp#L197

Like if that static global assignment doesn’t run before surge does surge won’t work

can you somehow force lto to not skip that? Or put a debug in the constructor of that connector object and see if it is hit?

[falkTX]

That should be fine. Other modules have similar globals so that is not an issue. This seems to me like a real buffer overflow, the code it points to does a wrong snprintf of a buffer with the size of that same buffer, which can result of it never writing the null ending byte.

I am doing some tests with changing it to

char prefix[PREFIX_SIZE+1] = {};

which should bypass the problem. LTO builds take a long time, so there is a break in between any change and its testing :tea: :sleeping:

will let you know if I find anything useful

[baconpaul]

Ahh interesting

I thought we moved to +1ing all our strings but we could have missed one indeed - I’ll peek when I’m back too

[falkTX]

Another small thing, get_prefix(char* txt, ...) uses snprintf(txt, TXT_SIZE, "%c_%s", 'a' + scene - 1, prefix); but the code that calls this function only has a stack array of 16 in size which is much smaller than TXT_SIZE.

So I am also trying

char prefix[TXT_SIZE+1];
get_prefix(prefix, ctrlgroup, ctrlgroup_entry, scene);

[baconpaul]

Yeah and 16 “should be” enough size for the result (that makes a string which is at most “a send” or some such) but if you end up with an optimized snprintf that assume it has the size that could go blammo. Good find

[baconpaul]

And as you can see our journey to go from 2003 ansi c strings to c++20 format library is a journey whcih is incomplete

[falkTX]

And that was it. Now LTO builds run nicely too. PR for surge (non-rack) coming up!

[baconpaul]

Wahey great find thank tou

[falkTX]

so this was an issue in surge, fixed by https://github.com/surge-synthesizer/surge/pull/6728