Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't attempt to copy anchors into NULL font #5405

Merged
merged 1 commit into from Apr 20, 2024

Conversation

Tynach
Copy link
Contributor

@Tynach Tynach commented 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 #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().

Type of change

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()`.
Copy link
Contributor

@skef skef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer the assert() in the function but I guess I can live without it. Approving.

@Tynach
Copy link
Contributor Author

Tynach commented Apr 19, 2024

I'd prefer the assert() in the function but I guess I can live without it. Approving.

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 assert(). I've heard of it and know roughly what it does, but I've never used it myself.

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.

@Tynach
Copy link
Contributor Author

Tynach commented Apr 20, 2024

@skef, what's the next step toward getting this merged, since I'm not authorized to merge it myself?

@skef skef merged commit fe39b7b into fontforge:master Apr 20, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants