Skip to content

Avoid crashes in Python scripts when objects are accessed in invalid state#5483

Merged
skef merged 23 commits intofontforge:masterfrom
iorsh:invalid_py_obj
Jan 1, 2025
Merged

Avoid crashes in Python scripts when objects are accessed in invalid state#5483
skef merged 23 commits intofontforge:masterfrom
iorsh:invalid_py_obj

Conversation

@iorsh
Copy link
Copy Markdown
Contributor

@iorsh iorsh commented Oct 25, 2024 •
edited
Loading

When the font is closed or a specific glyph is deleted in a Python script, there could still be Python objects around which reference the deleted C object. This causes segmentation faults in many scenarios.

We add checks and references which allow to check C object validity before performing operations.

Fixes #4615, fixes #5479.

iorsh added 19 commits October 20, 2024 02:44
@iorsh
Copy link
Copy Markdown
Contributor Author

iorsh commented Oct 25, 2024

The diff in python.c is infinite, but the most interesting stuff is as follows:

  • PyFF_FreeFV() - line 20343, invalidating the unique PyFF_Font object by setting its SplineFont *fv pointer to NULL.
  • PyFF_FreeSC() - line 20357, invalidating the unique PyFF_Glyph object by setting its SplineChar *sc pointer to NULL
  • splineuti.c:6001 - invalidate the Python object when SplineChar is destructed.
  • PyFF_Glyph_GetSC() - line 479, this function is the only way to get SplineChar* from PyFF_Glyph, and it raises exception whenever the glyph is invalid.
  • struct glyph_accessors_ - line 8093, introduces new internal getter/setter calling sequence for PyFF_Glyph, which receives SplineChar* as a guaranteed non-NULL argument.
  • PyFF_Glyph_set/get_wrapper() - line 10155, wrappers which implement Python standard getter/setter calling sequence, verify PyFF_Glyph and call internal accessors only when the glyph is valid.
  • setup_glyph_type() - line 10172, finalizes the PyFF_GlyphType with wrapped getters/setters.

@iorsh iorsh mentioned this pull request Oct 25, 2024
Merged
@iorsh
Copy link
Copy Markdown
Contributor Author

iorsh commented Oct 26, 2024

Ready for review now.

@skef
Copy link
Copy Markdown
Contributor

skef commented Nov 11, 2024

Sorry, I want to confirm the reference counting in this code and it's going to take me a while.

skef
skef approved these changes Jan 1, 2025
View reviewed changes
Copy link
Copy Markdown
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 didn't check this for completeness but what's here looks good to me and barring any bugs that should be easy to fix will make things better. Good work!

Comment thread fontforge/python.c Outdated
(char *)"a tuple of all anchor points in the glyph", NULL},
typedef struct glyph_accessors_ {
const char* name;
/* SlpineChar* argument is guaranteed non-NULL */
Copy link
Copy Markdown
Contributor

@skef skef Jan 1, 2025

Choose a reason for hiding this comment

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

typo

@skef skef merged commit 2e5bbdc into fontforge:master Jan 1, 2025
@iorsh iorsh deleted the invalid_py_obj branch March 28, 2025 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@skef skef skef approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

2 participants

@iorsh @skef