Re: Fix for segfault in Font_metric::mark_smob (optimization; Debian bug #684817)

[David Kastrup]

Don Armstrong <address@hidden> writes:

> On Fri, 28 Dec 2012, David Kastrup wrote:
>> Don Armstrong <address@hidden> writes:
>> > The attached patch fixes a segfault in mark_smob where s gets
>> > optimized away and garbage collected before m goes out of scope,
>> > leading to a segfault.
>> >
>> > There may be other cases of this bug floating around, but I didn't
>> > look particularly hard for them.
>> 
>> > diff --git a/lily/font-metric.cc b/lily/font-metric.cc
>> > index 6990afe..8ca88ef 100644
>> > --- a/lily/font-metric.cc
>> > +++ b/lily/font-metric.cc
>> > @@ -106,6 +106,9 @@ Font_metric::mark_smob (SCM s)
>> >  {
>> >    Font_metric *m = (Font_metric *) SCM_CELL_WORD_1 (s);
>> >    m->derived_mark ();
>> > +  // we must do this to avoid s being optimized out and garbage
>> > +  // collected; leading to a segfault above.
>> > +  scm_remember_upto_here_1(s);
>> >    return m->description_;
>> >  }
>> 
>> That makes precious little sense: mark_smob is only called while garbage
>> collection is in progress, so garbage collection can't be triggered in
>> its middle.
>
> The backtrace for this bug is here: 
> http://bugs.debian.org/cgi-bin/bugreport.cgi?msg=55;filename=lilypond-segfault.txt;att=1;bug=684817
>
> And the attached patch does fix it; however, it's possible that this
> is just because the patch obfuscates the actual bug. I'm certainly not
> an expert in guile nor lilypond internals at all.

More likely there is a difference in behavior because you recompiled
with different options in order to facilitate debugging or used a
different compiler.

Or the added code caused different memory layout.  At any rate, the
patch does not make sense.  The bug report also is for LilyPond 2.14.2.
A possible fix was committed as

commit 13fc2437e2aaa9ec5a65926dcb54d233a4797f45
Author: Joe Neeman <address@hidden>
Date:   Fri Jul 15 09:56:28 2011 +1000

    Fix a segfault caused by improper initialization of SCM.

diff --git a/lily/pango-font.cc b/lily/pango-font.cc
index a4fea75..26f2e53 100644
--- a/lily/pango-font.cc
+++ b/lily/pango-font.cc
@@ -47,6 +47,10 @@ Pango_font::Pango_font (PangoFT2FontMap *fontmap,
                        PangoFontDescription const *description,
                        Real output_scale)
 {
+  // This line looks stupid, but if we don't initialize physical_font_tab_ befo
+  // we allocate memory in scm_c_make_hash_table, then that could trigger a gar
+  // collection.
+  physical_font_tab_ = SCM_EOL;
   physical_font_tab_ = scm_c_make_hash_table (11);
   PangoDirection pango_dir = PANGO_DIRECTION_LTR;
   context_ = pango_context_new ();


in version 2.15.6.

-- 
David Kastrup