Issue 4339047: Fixes segfault in beam quanting. (Closed)
|
Created:
13 years, 1 month ago by MikeSol Modified:
13 years, 1 month ago CC:
lilypond-devel_gnu.org Visibility:
Public. |
DescriptionFixes segfault in beam quanting.
Patch Set 1 #Patch Set 2 : Adds programming error. #
Total comments: 1
MessagesTotal messages: 22
Quick-n-dirty fix for segfault in issue 1585. Cheers, MS
Sign in to reply to this message.
maybe it should be unquanted_x instead? The output doesn't look like anything
remotely resembling the test case:
\version "2.13.56"
{
\override Voice . NoteHead #'stencil = ##f
\repeat unfold 4 { d16 }
}
Sign in to reply to this message.
There is no unquanted_x - the x span is calculated earlier upstream, and the
quanting acts purely on y values.
My guess is that Lily does not know how much room to put between stems and just
kinda gives up. If you look at:
{
\override Voice . NoteHead #'stencil = ##f
\autoBeamOff
\repeat unfold 4 { d16 }
}
, which never touches the beaming code, you'll see the same ugly solution of
everything getting pushed to the right of the page.
I think the best solution may be to use this patch and put a note in the docs
warning users to create a tiny stencil (ie (make-filled-box '(0 . 0.001) '(0 .
001))) and then make it transparent if they want headless stems whose noteheads
don't shift stems depending on their direction. My workaround for the current
piece I'm working on has been to use ghosted mensural notation, which places the
stem in the middle of the notehead. So, when the stem changes direction, it
does not move to the left or right.
Sign in to reply to this message.
On Sat, Apr 2, 2011 at 3:34 PM, <mtsolo@gmail.com> wrote: > Quick-n-dirty fix for segfault in issue 1585. While this fix is prudent (I'd add a programming_error as well), it does feel like a kludge. The only way there could be no configuration at all is when generate_quants() rejects all of them if (!quant_range[d].contains (c->y[d])) I'm pretty sure using #f leads to infinity being used as a dimension somewhere. I'd suggest not trying #f as stencil callback for noteheads; If the head has no dimension, until where should its stem go? -- Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen
Sign in to reply to this message.
On Sat, Apr 2, 2011 at 10:37 PM, Han-Wen Nienhuys <hanwenn@gmail.com> wrote: >> Quick-n-dirty fix for segfault in issue 1585. > > While this fix is prudent (I'd add a programming_error as well), it > does feel like a kludge. The only way there could be no > configuration at all is when generate_quants() rejects all of them > > if (!quant_range[d].contains (c->y[d])) > > I'm pretty sure using #f leads to infinity being used as a dimension somewhere. > > I'd suggest not trying #f as stencil callback for noteheads; If the > head has no dimension, until where should its stem go? > I'm not sure what failure you guys are seeing. I'm getting warnings from the stem code, which does Real w = hed->extent (hed, X_AXIS)[dir]; and then lilypond: skyline.cc:111: void Building::precompute(Real, Real, Real, Real): Assertion `!isinf (slope_) && !isnan (slope_)' failed. Aborted (core dumped) -- Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen
Sign in to reply to this message.
On Apr 2, 2011, at 21:49, Han-Wen Nienhuys <hanwenn@gmail.com> wrote: > On Sat, Apr 2, 2011 at 10:37 PM, Han-Wen Nienhuys <hanwenn@gmail.com> wrote: > >>> Quick-n-dirty fix for segfault in issue 1585. >> >> While this fix is prudent (I'd add a programming_error as well), it >> does feel like a kludge. The only way there could be no >> configuration at all is when generate_quants() rejects all of them >> >> if (!quant_range[d].contains (c->y[d])) >> >> I'm pretty sure using #f leads to infinity being used as a dimension somewhere. >> >> I'd suggest not trying #f as stencil callback for noteheads; If the >> head has no dimension, until where should its stem go? >> > > I'm not sure what failure you guys are seeing. I'm getting warnings > from the stem code, which does > > Real w = hed->extent (hed, X_AXIS)[dir]; > > and then > > lilypond: skyline.cc:111: void Building::precompute(Real, Real, Real, > Real): Assertion `!isinf (slope_) && !isnan (slope_)' failed. > Aborted (core dumped) > Weird...I'm not sure why ours would segfault in one place whereas yours would hit an assertion error in another. You're right that the solution is to not use ##f as a stencil. My logic in doing so was trying to create up and down headless stems that lined up horizontally (a transparent notehead shifts these stems by the extent of the notehead). I realize now that there are other ways to do this, so I changed my piece. This bug is listed as critical under the assumption that there should be no segfaults in lilypond resulting from 3 lines of code (lily segfaults all the time when I accidentally feed her a PDF file instead of a .ly file, but I don't consider this to be a bug). However, if you feel that it should be downgraded to high, I could do that as well. In this case, it seems that the ideal behavior would be to have the stems reach down to the asymptotic point to which stems converge as their note heads get smaller and smaller (try overriding the notehead stencil to be progressively smaller boxes and you'll wee what I mean). I have no clue where in the code this would need to be implemented, though. I can do this task if no one else can, but I'm currently slammed w teaching, moving, and composing for a series of gigs, so if someone else has the hour or two it'd take to propose a non-kludgy solution (I agree that mine is a kludge), please do! Cheers, MS
Sign in to reply to this message.
On Sat, Apr 02, 2011 at 10:06:43PM -0400, mike@apollinemike.com wrote: > Weird...I'm not sure why ours would segfault in one place whereas yours > would hit an assertion error in another. Well, I didn't enable debug stuff in configure, so I'm certain why *I* didn't see any assertion errors. :) > This bug is listed as critical under the assumption that > there should be no segfaults in lilypond resulting from 3 lines of code > (lily segfaults all the time when I accidentally feed her a PDF file > instead of a .ly file, but I don't consider this to be a bug). However, if > you feel that it should be downgraded to high, I could do that as well. I think it should be downgraded to low. If we allow scheme, then it's impossible to make lilypond always produce either a message or output (the famous halting problem). I admit that this particular example uses an override rather than explicit scheme, but I think the same principle applies -- the user is trying to do something weird/unusual, so let's not panic when things go beserk. Let's compromise on Medium. :) > In this case, it seems that the ideal behavior would be to have the stems > reach down to the asymptotic point to which stems converge as their note > heads get smaller and smaller (try overriding the notehead stencil to be > progressively smaller boxes and you'll wee what I mean). Agreed. > I have no clue > where in the code this would need to be implemented, though. I can do this > task if no one else can, but I'm currently slammed w teaching, moving, and > composing for a series of gigs, so if someone else has the hour or two > it'd take to propose a non-kludgy solution (I agree that mine is a > kludge), please do! I'll move it into an enhacement in the mean-time. Cheers, - Graham
Sign in to reply to this message.
> (lily segfaults all the time when I accidentally feed her a PDF file
> instead of a .ly file, but I don't consider this to be a bug)
I do. Any user program *must not* produce a segfault IMHO if fed with
user data, regardless of its origin.
While developing FreeType, various people (including myself) used
fuzzers to create invalid fonts which are then read by FreeType. This
led to the discovery of a few dozen bugs...
Perhaps something similar can be done for LilyPond also. You might
have a look at the `ftrandom' utility program in the source code of
FreeType; it should be rather straightforward to adapt it.
Werner
Sign in to reply to this message.
Werner LEMBERG wrote Sunday, April 03, 2011 7:16 AM >> (lily segfaults all the time when I accidentally feed her a PDF >> file >> instead of a .ly file, but I don't consider this to be a bug) > > I do. Any user program *must not* produce a segfault IMHO if fed > with > user data, regardless of its origin. +1 > While developing FreeType, various people (including myself) used > fuzzers to create invalid fonts which are then read by FreeType. > This > led to the discovery of a few dozen bugs... > > Perhaps something similar can be done for LilyPond also. You > might > have a look at the `ftrandom' utility program in the source code > of > FreeType; it should be rather straightforward to adapt it. This or some alternative sounds like a fun project for 2.15. Trevor
Sign in to reply to this message.
On Sun, Apr 03, 2011 at 08:16:12AM +0200, Werner LEMBERG wrote: > > > (lily segfaults all the time when I accidentally feed her a PDF file > > instead of a .ly file, but I don't consider this to be a bug) > > I do. Any user program *must not* produce a segfault IMHO if fed with > user data, regardless of its origin. It it possible to make guile crash? Honest question here; I don't know. Maybe something about scheme's function-language-ness means that it is impossible to make it crash...? I mean, I'm sure that there are bugs in the guile-2.0 interpreter, which would hopefully be fixed if we discovered any. But doesn't guile have some C compatibility stuff? Is there any way to write a guile program that gets a null pointer and then tries to use it? If so, then logically we can't make lilypond never crash. Cheers, - Graham
Sign in to reply to this message.
>> I do. Any user program *must not* produce a segfault IMHO if fed
>> with user data, regardless of its origin.
>
> It it possible to make guile crash?
Maybe. However, with `crash' I mean that lilypond aborts with a
segfault or something similar. It's quite easy to write an endless
loop or to exhaust the memory, but in the former case lilypond's guile
interpreter just hangs, and you should be able to abort with ^C, and
in the latter case lilypond should abort with a proper (Guile) error
message, and maybe we can add some measures to prevent unplausible
memory allocations.
> Is there any way to write a guile program that gets a null pointer
> and then tries to use it?
AFAIK, this is not possible in Guile since it is an interpreted
language.
Werner
Sign in to reply to this message.
On Apr 3, 2011, at 2:17 PM, Werner LEMBERG wrote: > >>> I do. Any user program *must not* produce a segfault IMHO if fed >>> with user data, regardless of its origin. >> >> It it possible to make guile crash? > > Maybe. However, with `crash' I mean that lilypond aborts with a > segfault or something similar. It's quite easy to write an endless > loop or to exhaust the memory, but in the former case lilypond's guile > interpreter just hangs, and you should be able to abort with ^C, and > in the latter case lilypond should abort with a proper (Guile) error > message, and maybe we can add some measures to prevent unplausible > memory allocations. > >> Is there any way to write a guile program that gets a null pointer >> and then tries to use it? > > AFAIK, this is not possible in Guile since it is an interpreted > language. > > > Werner I'll chime in here and say that I am still for applying my patch to beam quanting as a general fix. I agree that refining how stems meet up w/ noteheads is a better solution, but I think the bigger problem lies in the fact that beam quanting will result in a segfault any time it finds no good solutions, which arises here but could also arise in other unforeseeable ways. The best way to handle this, then, is a programming error followed by returning the best possible value which, in this case, is unquanted_y. New patch set at http://codereview.appspot.com/4339047 Cheers, MS
Sign in to reply to this message.
mike@apollinemike.com wrote Sunday, April 03, 2011 7:31 PM > I'll chime in here and say that I am still for > applying my patch to beam quanting as a general fix. > I agree that refining how stems meet up w/ noteheads > is a better solution, but I think the bigger problem > lies in the fact that beam quanting will result in a > segfault any time it finds no good solutions, which > arises here but could also arise in other unforeseeable > ways. The best way to handle this, then, is a > programming error followed by returning the best > possible value which, in this case, is unquanted_y. LGTM > New patch set at http://codereview.appspot.com/4339047 > Cheers, > MS Trevor
Sign in to reply to this message.
On Sun, Apr 3, 2011 at 3:17 PM, Werner LEMBERG <wl@gnu.org> wrote: >>> I do. Any user program *must not* produce a segfault IMHO if fed >>> with user data, regardless of its origin. >> >> It it possible to make guile crash? > > Maybe. However, with `crash' I mean that lilypond aborts with a > segfault or something similar. It's quite easy to write an endless > loop or to exhaust the memory, but in the former case lilypond's guile > interpreter just hangs, and you should be able to abort with ^C, and > in the latter case lilypond should abort with a proper (Guile) error > message, and maybe we can add some measures to prevent unplausible > memory allocations. LilyPond exposes large parts of the internal implementation through the Scheme interface, and that has as a side-effect that there are many ways for users to break lilypond. This is unlikely to lead to arbitrary behavior, as Guile values themselves themselves are type tagged. The worst which can happen is that a value is incorrectly type-cast which leads to either a null dereference or some other type assertion. I don't think it is productive to try to systematically plug all these errors; at best, you'll replace a bunch of segmentation faults with just as unhelpful assertion failures. -- Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen
Sign in to reply to this message.
> LilyPond exposes large parts of the internal implementation through
> the Scheme interface, and that has as a side-effect that there are
> many ways for users to break lilypond. This is unlikely to lead to
> arbitrary behavior, as Guile values themselves themselves are type
> tagged. The worst which can happen is that a value is incorrectly
> type-cast which leads to either a null dereference or some other
> type assertion.
Could you give a Scheme example for that, please?
> I don't think it is productive to try to systematically plug all
> these errors; at best, you'll replace a bunch of segmentation faults
> with just as unhelpful assertion failures.
I definitely prefer assertions to segfaults. But maybe that's only
me.
Werner
Sign in to reply to this message.
On Mon, Apr 04, 2011 at 05:22:28AM +0200, Werner LEMBERG wrote:
>
> > I don't think it is productive to try to systematically plug all
> > these errors; at best, you'll replace a bunch of segmentation faults
> > with just as unhelpful assertion failures.
>
> I definitely prefer assertions to segfaults. But maybe that's only
> me.
I agree -- even something as "unhelpful" as
{
program_error("Oops, I'm confused.");
// clean up memory?
...
exit(1);
}
is still better than a segfault. But I think there's a problem of
scale here.
I'm quite comfortable taking a hard line on any non-tweak
segfaults -- if you have a file which does not contain any #
symbols or [ \override \tweak \overrideProperty \set ] strings, it
Should Not Segfault (tm).
(I may have forgotten to list one or two ways of tweaking the
output, so the above list might be expanded)
Cheers,
- Graham
Sign in to reply to this message.
On Mon, Apr 4, 2011 at 12:22 AM, Werner LEMBERG <wl@gnu.org> wrote: > >> LilyPond exposes large parts of the internal implementation through >> the Scheme interface, and that has as a side-effect that there are >> many ways for users to break lilypond. This is unlikely to lead to >> arbitrary behavior, as Guile values themselves themselves are type >> tagged. The worst which can happen is that a value is incorrectly >> type-cast which leads to either a null dereference or some other >> type assertion. > > Could you give a Scheme example for that, please? Typical examples: * scm_cdr(SCM_EOL) This basically dereferences an (almost) null pointer. Possibly, this crashes neatly in debug mode (I'm not sure). The SCM_CDR() variant will surely crash with segmentation fault. * unsmob_grob(x)->foo() If x is not a grob, unsmob_grob(x) returns NULL. Boom. >> I don't think it is productive to try to systematically plug all >> these errors; at best, you'll replace a bunch of segmentation faults >> with just as unhelpful assertion failures. > > I definitely prefer assertions to segfaults. But maybe that's only > me. It does not make any difference: you'll need to start up a debugger to figure what's going on in either case. -- Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen
Sign in to reply to this message.
>>> The worst which can happen is that a value is incorrectly
>>> type-cast which leads to either a null dereference or some other
>>> type assertion.
>>
>> Could you give a Scheme example for that, please?
>
> Typical examples:
>
> * scm_cdr(SCM_EOL)
>
> This basically dereferences an (almost) null pointer. Possibly,
> this crashes neatly in debug mode (I'm not sure). The SCM_CDR()
> variant will surely crash with segmentation fault.
There is a misunderstanding. I was rather talking about Scheme code
entered by the user. The above is a programming error, isn't it?
> * unsmob_grob(x)->foo()
>
> If x is not a grob, unsmob_grob(x) returns NULL. Boom.
This looks like a programming error too...
I don't understand why you think that such situations shouldn't be
fixed in the source code.
Werner
Sign in to reply to this message.
On Mon, Apr 4, 2011 at 12:43 AM, Werner LEMBERG <wl@gnu.org> wrote: >> This basically dereferences an (almost) null pointer. Possibly, >> this crashes neatly in debug mode (I'm not sure). The SCM_CDR() >> variant will surely crash with segmentation fault. > > There is a misunderstanding. I was rather talking about Scheme code > entered by the user. The above is a programming error, isn't it? Yes and no. If a user passes '() (ie. SCM_EOL), into a variable which then is interpreted by the C++ code as a pair, then we get a crash. >> * unsmob_grob(x)->foo() >> >> If x is not a grob, unsmob_grob(x) returns NULL. Boom. > > This looks like a programming error too... > > I don't understand why you think that such situations shouldn't be > fixed in the source code. I think it is good that these are fixed, but not important enough to spend serious time on finding and plugging all of them. The question is how much of the code we should consider user-serviceable. If one C++ part of Lily passes data using Scheme types to another C++ part, should that other part be resistent users inserting bogus values into that internal channel ? -- Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen
Sign in to reply to this message.
Han-Wen Nienhuys wrote Monday, April 04, 2011 6:13 AM > I think it is good that these are fixed, but not important enough > to > spend serious time on finding and plugging all of them. The > question > is how much of the code we should consider user-serviceable. If > one > C++ part of Lily passes data using Scheme types to another C++ > part, > should that other part be resistent users inserting bogus values > into > that internal channel ? Can we distinguish code that is publicly accessible in Scheme? Or maybe those routines that are advertised in the docs. If so, it is these that need to be robust. I would not be worried by segfaults in anything that is accessible only via a local build. Anyone building LP can surely handle segfaults themselves. But users struggling with Scheme need all the help we can provide. Trevor
Sign in to reply to this message.
On Apr 4, 2011, at 5:41 AM, Trevor Daniels wrote: > > Han-Wen Nienhuys wrote Monday, April 04, 2011 6:13 AM > >> I think it is good that these are fixed, but not important enough to >> spend serious time on finding and plugging all of them. The question >> is how much of the code we should consider user-serviceable. If one >> C++ part of Lily passes data using Scheme types to another C++ part, >> should that other part be resistent users inserting bogus values into >> that internal channel ? > > Can we distinguish code that is publicly accessible in Scheme? Or > maybe those routines that are advertised in the docs. If so, it is these > that need to be robust. I would not be worried by segfaults in anything > that is accessible only via a local build. Anyone building LP can surely > handle segfaults themselves. But users struggling with Scheme need > all the help we can provide. > > Trevor > I've gotten one LGTM on the 3-ish line patch patch I proposed. Could I please have either a second "LGTM" or a "please don't push." http://codereview.appspot.com/4339047/ I agree with Trevor that, for common tweaks (i.e. ones advertised in the docs), LilyPond shouldn't segfault. Cheers, MS
Sign in to reply to this message.
lgtm http://codereview.appspot.com/4339047/diff/10001/lily/beam-quanting.cc File lily/beam-quanting.cc (right): http://codereview.appspot.com/4339047/diff/10001/lily/beam-quanting.cc#newcod... lily/beam-quanting.cc:462: if (!configs.size()) * move to directly after generate_quants() * use configs.empty()
Sign in to reply to this message.
|
