Don't attempt to copy anchors into NULL font#5405
Merged
skef merged 1 commit intofontforge:masterfrom Apr 20, 2024
Merged
Conversation
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
approved these changes
Apr 18, 2024
Contributor
skef
left a comment
There was a problem hiding this comment.
I'd prefer the assert() in the function but I guess I can live without it. Approving.
Contributor
Author
I'll be honest, I had the code already written and branched, and just had to type up a commit description.. And I've yet to look up how to use I'd be more than willing to add it if you want, but that'd take extra time for me to look up its proper usage and whatnot. Figured it might be best left to someone more experienced. |
Contributor
Author
|
@skef, what's the next step toward getting this merged, since I'm not authorized to merge it myself? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Any time
SFDDumpUndo()orSFDGetUndo()are run, they end up callingSplineCharCopy()withNULLfor theSplineFont *intoparameter. When this happens,SplineCharCopy()attempts to copy the anchors into the new SplineChar'sparentfont, but since that is set toNULLit crashes whenAnchorPointsDuplicate()attempts to read fromsc->parent->anchor.Because
SFDDumpUndo()andSFDGetUndo()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 whenintois set toNULL.To do this, instead of setting
nsc->anchorto the output returned byAnchorPointsDuplicate(), I use a ternary statement to detect whether or notintois NULL. If it is, thennsc->anchoris set to NULL, and otherwise the value returned byAnchorPointsDuplicate()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 ifintoisNULL, 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 furtherAnchorPointcrashes.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==NULLbefore doing anything with theAnchorPointsallows all the hint-related stuff thatSplineCharCopy()does continue to work for the sake of bothSFDDumpUndo()andSFDGetUndo().Type of change
Fixes FontForge crashes when a font has anchor classes, and you perform bulk operations on the font (such as changing the EM size) #5130