FontForge crashes when a font has anchor classes, and you perform bulk operations on the font (such as changing the EM size) #5130
Labels
Comments
Tynach
added a commit
to Tynach/fontforge
that referenced
this issue
Apr 11, 2024
When bulk operations are performed on a font, FontForge runs `AnchorPoint *AnchorPointsDuplicate(AnchorPoint *base, SplineChar *sc)`. Early in the function there is a `for` loop which runs while `base` isn't NULL, but it later accesses `sc->parent->anchor` without checking if `sc->parent` itself is NULL first. This seems to be because the function assumes it will only be called when the duplication of anchor points is actually desired, and thus there should be a valid place to copy the anchor points to. Instead, it's being called from within `SplineChar *SplineCharCopy(SplineChar *sc, SplineFont *into, struct sfmergecontext *mc)`, which is itself being called with `into` set to 0 within 2 different cases handled by `void SFDDumpUndo(FILE *sfd, SplineChar *sc, Undoes *u, const char *keyPrefix, int idx)`, and also once within `Undoes *SFDGetUndo(FILE *sfd, SplineChar *sc, const char *startTag, int current_layer)`. `SFDDumpUndo()` seems to only call `SplineCharCopy` as a means toward some of its side effects. It appears to make a new empty SplineChar copy the SplineChar it's working on into the new SplineChar, extract and dump hints from it, and then delete the SplineChar it created. Likewise, `SFDGetUndo()` seems to also only call `SplineCharCopy()` to do some temporary managing of hint information. Since these appear to be the only scenarios where `SplineCharCopy` is called with `into` set to NULL, and the only thing that is being done in those situations is do some stuff relating to font hints, I've determined that it's safe to simply not duplicate the AnchorPoints when sc->parent is set to NULL. This fixes fontforge#5130, and indeed is just the same fix I proposed back then. At the time I wasn't confident this was safe, but I've extensively used FontForge with this change made and had no further AnchorPoint crashes. I also further investigated the root causes, which I describe above. A more 'proper' fix would probably include systematic changes to how hint undoes are saved and loaded from SFD files, but that's beyond my skill and, quite honestly, not something I even care about that much. The code appears to work despite it's oddities, and simply checking if `sc->parent` is NULL before doing anything with the AnchorPoints allows all the hint-related stuff that `SplineCharCopy()` does continue to work for the sake of both `SFDDumpUndo()` and `SFDGetUndo()`.
Tynach
added a commit
to Tynach/fontforge
that referenced
this issue
Apr 18, 2024
Any time `SFDDumpUndo()` or `SFDGetUndo()` are run, they end up calling `SplineCharCopy()` with `NULL` for the `SplineFont *into` parameter. When this happens, `SplineCharCopy()` attempts to copy the anchors into the new SplineChar's `parent` font, but since that is set to `NULL` it crashes when `AnchorPointsDuplicate()` attempts to read from `sc->parent->anchor`. Because `SFDDumpUndo()` and `SFDGetUndo()` appear to only do this so that they can have a copy of the character's hints at a given undo state (and then copy thoes hints into or out of the .sfd file), never affect anchor classes, and delete the parentless SplineChar immediately after, I've determined that it's safe to simply not duplicate the AnchorPoints when `into` is set to `NULL`. To do this, instead of setting `nsc->anchor` to the output returned by `AnchorPointsDuplicate()`, I use a ternary statement to detect whether or not `into` is NULL. If it is, then `nsc->anchor` is set to NULL, and otherwise the value returned by `AnchorPointsDuplicate()` is used. I decided not to do the reverse (which could have made the code shorter) simply because line 505 already includes a similar ternary statement that checks if `into` is `NULL`, and I decided to use the same format. This fixes fontforge#5130, and is a slightly more elegant variation of the same fix that I had proposed back then (more elegant because I skip the entire `AnchorPointsDuplicate()` function call instead of merely skipping over the loop that makes up the majority of the function). I don't see a reason for this to be functionally any different from the originally proposed solution, and I've extensively used FontForge with that change made with no further `AnchorPoint` crashes. A more 'proper' fix would probably include systematic changes to how hint undoes are saved and loaded from SFD files, but that's beyond my skill and, quite honestly, not something I even care about that much. That code appears to work despite it's oddities, and simply checking if `into==NULL` before doing anything with the `AnchorPoints` allows all the hint-related stuff that `SplineCharCopy()` does continue to work for the sake of both `SFDDumpUndo()` and `SFDGetUndo()`.
skef
pushed a commit
that referenced
this issue
Apr 20, 2024
Any time `SFDDumpUndo()` or `SFDGetUndo()` are run, they end up calling `SplineCharCopy()` with `NULL` for the `SplineFont *into` parameter. When this happens, `SplineCharCopy()` attempts to copy the anchors into the new SplineChar's `parent` font, but since that is set to `NULL` it crashes when `AnchorPointsDuplicate()` attempts to read from `sc->parent->anchor`. Because `SFDDumpUndo()` and `SFDGetUndo()` appear to only do this so that they can have a copy of the character's hints at a given undo state (and then copy thoes hints into or out of the .sfd file), never affect anchor classes, and delete the parentless SplineChar immediately after, I've determined that it's safe to simply not duplicate the AnchorPoints when `into` is set to `NULL`. To do this, instead of setting `nsc->anchor` to the output returned by `AnchorPointsDuplicate()`, I use a ternary statement to detect whether or not `into` is NULL. If it is, then `nsc->anchor` is set to NULL, and otherwise the value returned by `AnchorPointsDuplicate()` is used. I decided not to do the reverse (which could have made the code shorter) simply because line 505 already includes a similar ternary statement that checks if `into` is `NULL`, and I decided to use the same format. This fixes #5130, and is a slightly more elegant variation of the same fix that I had proposed back then (more elegant because I skip the entire `AnchorPointsDuplicate()` function call instead of merely skipping over the loop that makes up the majority of the function). I don't see a reason for this to be functionally any different from the originally proposed solution, and I've extensively used FontForge with that change made with no further `AnchorPoint` crashes. A more 'proper' fix would probably include systematic changes to how hint undoes are saved and loaded from SFD files, but that's beyond my skill and, quite honestly, not something I even care about that much. That code appears to work despite it's oddities, and simply checking if `into==NULL` before doing anything with the `AnchorPoints` allows all the hint-related stuff that `SplineCharCopy()` does continue to work for the sake of both `SFDDumpUndo()` and `SFDGetUndo()`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
This caused me to lose a significant amount of work on what's basically my first font project, because at some point one of the bulk operations happened while saving the file, which corrupted it. I believe it was trying to auto-hint glyphs that were composed of references to other glyphs, though I'm not 100% sure of that.
To reproduce it with the current
masterbranch, simply open up any popular multilingual font (Roboto is the one I mostly tested on, but this happens with Noto Sans and several others as well; basically, any font that supports Unicode's 'combining diacritics', with anchor marks set up so they work on a variety of characters), open up 'Element' → 'Font Info...', go to the 'General' page, and then change the EM size (I typically tried doubling or halving the EM size; results were the same). Make sure that 'Scale Outlines' is enabled/checked, and click 'OK' at the bottom of the window to experience the crash.If you have a glyph that has both its own anchor classes, as well as references to other glyphs with those same anchor classes defined (or maybe just anchor classes in general, but I didn't test that), the crash happens much more often. Simply opening the 'Anchor Control' window for an anchor class, and then within it choosing to view any other glyph (doesn't matter if it's a mark or base glyph), will trigger the same crash. The backtrace in that case is usually longer, but the lines of code causing the crash are the same.
On the topic of backtraces, here's a couple of examples:
The culprit seems to be that within
AnchorPointsDuplicate(),sc->parentis sometimes set toNULL, causingsc->parentto be at address0x300, which causes a segfault when an attempt is made to setactoo its value. I've studied the code around it, and think that what's happening is that onlybaseis tested for aNULLvalue, because it's assumed that if it's notNULL, thensc->parentwill also be valid.I've tried changing the outer
forloop to read:And this seems to fix this particular crash.. But I don't know if it could potentially lead to some data not being copied over when copying from one font to another. I've done some testing, and an error dialog box pops up if I try to just paste glyphs from one font into another font, but if I remake the anchor classes within the new font and then try pasting again it appears to work fine.
Trying to use 'Edit' → 'Copy Lookup Data' in one font, and then using 'Paste' on another font, crashes still... But it crashed before, still crashes, and does not seem to be a related crash.
I'd open a pull request, except I'm not confident in my fix. I don't know this codebase nearly well enough to know exactly what
sc->parentis usually used for, or how it's supposed to be set.Within the calling function
SplineCharCopy(), there's the linensc->parent = into;- and when I have a font that doesn't have glyphs containing both anchor marks and references to glyphs with those same anchor marks, I've used breakpoints to confirm that in that case bothnsc->anchorandinto(and thusnsc->parent) are set toNULLwhen that function callsAnchorPointsDuplicate(). This leads me to suspect that it's quite likely that a real fix would involve making sure that thescpassed toSplineCharCopyhassc->anchorset toNULLin the situations where this is happening... But I don't know enough about this codebase to have any idea where to look for that.Edit: I forgot to mention that I'm using Linux, specifically KDE Neon, and compiled the source code from the 'master' branch. I don't think that changes anything, though.
The text was updated successfully, but these errors were encountered: