Spiro crash when merging points #3569
Comments
|
Another crash: Unfortunately I don't have a file for this one, but the bt should help debug the code because I was making the same action when it happened. |
|
After working with this for a while, and experiencing around twenty crashes, I've come to an observation. It's more likely to happen if you drag two points together directly then if you drop the point, then select it with the arrow tool and drag it together. |
|
I spent three fruitless hours today trying to figure this out with no luck. Obviously, just commenting out I tried...
I think I'm not (yet) skilled enough to fix this. Someone better than me will have to do it. |
|
This crash has completely and utterly defeated me. Today I thought, let's try Valgrind...I don't have much experience with it, but let's fire it up. So I did. And FontForge DOESN'T CRASH WITH MY TEST CASE UNDER VALGRIND, BUT CRASHES WITH GDB AND REGULARLY. I'm sorry to write in all caps, but that pill was hard to swallow, to put it mildly. Valgrind pukes out a lot, here's a snippet: I hate to name drop you constantly, but I'm wondering if @skef, @jtanx maybe even @JoesCat have ever seen this before and have any advice. I need someone with more experience debugging C code to lend a hand or give me some advice. |
|
OK maybe I have a lead. This patch leads to no crash: diff --git a/fontforge/sfd.c b/fontforge/sfd.c
index b6030f035..060026f82 100644
--- a/fontforge/sfd.c
+++ b/fontforge/sfd.c
@@ -3900,7 +3900,7 @@ static void SFDGetSpiros(FILE *sfd,SplineSet *cur) {
while ( fscanf(sfd,"%lg %lg %c", &cp.x, &cp.y, &cp.ty )==3 ) {
if ( cur!=NULL ) {
if ( cur->spiro_cnt>=cur->spiro_max )
- cur->spiros = realloc(cur->spiros,(cur->spiro_max+=10)*sizeof(spiro_cp));
+ cur->spiros = realloc(cur->spiros,(cur->spiro_max+=1000)*sizeof(spiro_cp));
cur->spiros[cur->spiro_cnt++] = cp;
}
}I got the idea from studying the Valgrind output, but |
|
Update: I have no idea what the right number is, but that line is definitely related to the crash. 😞 😭 |
|
The intention of the code is that the number doesn't matter as long as realloc succeeds. This shouldn't matter either, but what values do spiro_cnt and spiro_max have to start with? And can you verify the "incremental" realloc before the failure is successful (returns non-null)? |
|
Both are initialized to zero. I know it's weird that this matters, I really do. The code has so many problems and is written so bizarrely, with use-after-frees required in I did a diff --git a/fontforge/sfd.c b/fontforge/sfd.c
index b6030f035..1efe7500e 100644
--- a/fontforge/sfd.c
+++ b/fontforge/sfd.c
@@ -3901,12 +3901,18 @@ static void SFDGetSpiros(FILE *sfd,SplineSet *cur) {
if ( cur!=NULL ) {
if ( cur->spiro_cnt>=cur->spiro_max )
cur->spiros = realloc(cur->spiros,(cur->spiro_max+=10)*sizeof(spiro_cp));
+ if (cur->spiros == NULL) {
+ printf("Realloc failure");
+ }
cur->spiros[cur->spiro_cnt++] = cp;
}
}
if ( cur!=NULL && (cur->spiros[cur->spiro_cnt-1].ty&0x7f)!=SPIRO_END ) {
if ( cur->spiro_cnt>=cur->spiro_max )
cur->spiros = realloc(cur->spiros,(cur->spiro_max+=1)*sizeof(spiro_cp));
+ if (cur->spiros == NULL) {
+ printf("Realloc failure");
+ }
memset(&cur->spiros[cur->spiro_cnt],0,sizeof(spiro_cp));
cur->spiros[cur->spiro_cnt++].ty = SPIRO_END;
}
|
|
Oh, wait, do you know that the crash happens around that line. Because if not there's probably just a miscalculation downstream and the large amount of memory from the 1000 overcomes it. |
|
@skef Oh no, it doesn't crash here. It crashes at the free. But I know that changing this fixes it, so this line is at least related. |
I think Valgrind does a calloc(). From our point of view, you treat libspiro as a "blackbox" and nobody really knows "how many" points there are, but you "do know" results from spiros comes-out through bezctx_ff.c, and you require space for: At the moment, I can't follow-up with this since my computer is needing to be updated, so I haven't been doing anything fontforge GUI related in the last couple months. |
|
@JoesCat Thanks a lot, you've given me a lot to think about. This is concerning:
To me, this sounds like, even though everything is working correctly, if my merged Spiro spline has more than ten points, it will crash? That's bad. Any idea how we can stop using a constant, or is raising it enough? |
|
From our point of view, you treat libspiro as a "blackbox" and nobody
really knows "how many" points there are
No - it is not a worry. If you create a single spiro, you continue and
continue and continue, and when it makes the crazy spirals, it still goes
on without crashing. The crazy spirals is where you get the crazy number of
points, but it works ok. As a single Spiro, all this appears to work ok.
To me, this sounds like, even though everything is working correctly, if
my merged Spiro spline has more than ten points, it will crash? That's
bad. Any idea how we can stop using a constant, or is raising it
enough?
It comes back to the problem that even though it looks simply like you are
merging two spiros, you are actually making a third new spiro.
If you look at your crash.zip spiro file you're last points are "v" which
are angles, but if you look at the spiros, you'll see that the last point
is actually the 4 point curve "o". The believe the endpoint of an open
spiro will always be the "o" curve as per FontForge spiro creation, even
though you have "v" chosen in the menu. So your first problem here is you
are actually attempting to merge a curve to another endpoint. The solution
here is to probably look at what you have selected, and when creating this
third spiro, to use "v" or whatever the last menu selection is, and not
"o", because I think your intent for this example was to add another "v".
So, to begin with, you hold the two different spiros as temp values, and you
build the third spiro as a temp (I mention temps because this allows you to
show the third temp as a successful merge, or as a failed spiro with
straight lines if it fails to merge), if the user doesn't like this, they
can revert back. In terms of the pass/fail thing, I really don't like the
functions that return void, because you cannot return pass/fail, so I think
the CV merge functions will need to return pass/fail. On pass, you can
convert the third spiro to the new results, on fail, you keep the old two
spiros, so the user can try something else.
Now we also have another problem - spiro direction.
If you're joining the end point of one spiro to the start of the other,
you're ok, but if you're joining the start of one to the start of the
other, or end of one to the end of the other, then one of the spiros has to
change direction, and unfortunately the curve that changes direction is not
going to look the same.
This is the harder problem.
sort of got stuck on this problem with libspiro because users may flip
directions because they have one line inside another, like the letter "O".
The libspiro tests skip self-tests 10 and 11 more or less due to this
problem looking at it from another point of view.
I've thought about this direction change, and probably we'd have to tell
libspiro to output the results in reverse order, so I'd need to add another
start symbol in libspiro for this scenario, maybe call it "r" or something,
and then another symbol to reverse the reverse back to normal direction if
we merge something on the other end, so instead of "z" or "}" there needs
to be something else.
For now, my suggestion is look at the easier problem. you have two spiros
running in the same direction. Leave the harder problem for a later date.
Looking at your crash.zip,
The first value in both spiros, the "{" is actually an "o", but we use "{"
to signify the start of a curve. When you join the first spiro to the second
spiro. the last "o" x,y point disappears on the first spiro, and you need to
change the "{" into "o" when you create the third spiro.
So instead of "v"..."{" it merges as "v"..."o"
|
|
...one more item to add. You may want to put your crash spiro as data in libspiro call-test and run it in verbose mode to try out different ideas. In summary, it's more complicated than a simple merge. Hope all this info helps. |
|
I've been looking at this, and the spiro code is outright dodgy. I'm not sure though if that's a red herring, since the actual segfault seems to be when freeing the spline and not the spiros. But what that earlier patch to increase the |
|
Merging spline points is like pushing a rope. You don't want to be editing the results, what you want to do is edit the source information - The spline is supposed to be the result of the spiro calculations, so you should build a third spiro as the source for the new spiro. This might not look exactly the same, but may be close enough.... ...one more edit 'v' #2 becomes '{' since this is still an open spiro. |
|
Thank you for all the help guys. I haven't forgotten this, I'm just working on another patchset to replace #3617, which I hope to PR in a few hours. |
|
I don't think I'll be providing a quick solution to this any time soon, but just wanted to say - I'm pretty confident that this (and likely a few other spiro bugs) are caused by how that Fixing that will not be trivial. |
|
@jtanx Yeah, I've definitely seen. I might take a crack at it since I've got a few projects brewing that use Spiros. |
|
It's pretty rough, but this is where I got with this - jtanx@468d11d and jtanx@0d70729 It avoids the crash and valgrind errors, but the behaviour's still pretty weird. Whenever you close a spiro path and continue moving the points around, for some reason it adds a (0,0) point like: Although I sort of see similar behaviour on master. Maybe I'm just using it wrong 🤷 My change for lastselcp might be wrong too (since it won't pick up any side-effects of modifying the actual selected spiro point) |
|
Looking at your screenshot, it's got curves so you seem to have a successful spiro solution (straight lines would be failure). The (0,0) may be due to the spiro "z" which may be at the end of the string (0,0). You may want to try and see if libspiro "HEAD" is different. It probably may behave better. I ran into scaling problems and you might be running into similar issues as your xmin..xmax ymin..ymax change. It was a problem found when making one of the tests, and looking at the original spiro code, it appears spiro was really designed to work between -1..+1, not the -1000...+1000 or larger values we use in working with fonts. I made some modifications in libspiro that did rescaling in "Allow Spiros to be scaled and/or shifted. Scaling bug fixed." |
|
@jtanx I've never received a point at |
Interesting. #4064 addressed a similar symptom but that was quadratic-conversion-specific, and was always a control point, while that's clearly a cubic splineset and there's an on-curve point there. |
|
I was taking a closer look at libspiro to loop a,h (and also to see about resolving tests 10 and 11), and it appears there is a long standing bug since 2007. I've held-off on releasing the bugfixed libspiro because it can use an improved 'cyclic' loop. |
* Fix Spiro-related merge point crash Closes fontforge#3569. * Fix Spiro-related merge point crash Closes fontforge#1004.
Arch Linux, 3aabfd2
Reproduction steps:
gdbCould be related to #3568 and by association #1004, but different backtrace.
The text was updated successfully, but these errors were encountered: