Skip to content

fix segfault triggered by Python del c[i:j]#5352

Merged
skef merged 2 commits intofontforge:masterfrom
mf2vec-dev:fix-segfault-py-del-c
Jan 28, 2024
Merged

fix segfault triggered by Python del c[i:j]#5352
skef merged 2 commits intofontforge:masterfrom
mf2vec-dev:fix-segfault-py-del-c

Conversation

@mf2vec-dev
Copy link
Copy Markdown
Contributor

@mf2vec-dev mf2vec-dev commented Jan 25, 2024

This PR adresses a segmentation fault when running the following Python code:

import fontforge
c = fontforge.contour()
c.moveTo(0, 0).lineTo(10, 0).lineTo(20, 0).lineTo(30, 0)
del c[1:3]

With this change, a Python TypeError is raised instead of triggering the segmentation fault.

This is only a problem for contours. Layers (del l[i]) and Layer Info Arrays (del font.layers[i]) don't allow slicing.

Type of change

  • Bug fix

@iorsh
Copy link
Copy Markdown
Contributor

iorsh commented Jan 27, 2024

Could it be better to just support the del c[i:j]?
https://github.com/fontforge/fontforge/blob/master/fontforge/python.c#L2699 already has the code for deleting a single point, deleting a range shouldn't be much different.

iorsh
iorsh reviewed Jan 27, 2024
View reviewed changes
Comment thread fontforge/python.c Outdated
@mf2vec-dev
Reuse replacing by empty list to support deletion
4db07be 
Make del c[i:j] equivalent to c[i:j] = []
Add test for deleting points from contour
@mf2vec-dev
Copy link
Copy Markdown
Contributor Author

mf2vec-dev commented Jan 28, 2024

I'm now reusing the mechanism of c[i:j] = [] to make del c[i:j] work. Slice validation is also handled this way (assuming slice validation of c[i:j] = d is thoroughly tested). I've changed the error message to also mention slices as a valid subscript type.

iorsh
iorsh reviewed Jan 28, 2024
View reviewed changes
Comment thread tests/test1021.py
@iorsh
Copy link
Copy Markdown
Contributor

iorsh commented Jan 28, 2024

Looks good to me. Appveyor is broken, hopefully it will be back in a few hours. I'll check it.

skef
skef approved these changes Jan 28, 2024
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.

LGTM

@skef skef merged commit d500c55 into fontforge:master Jan 28, 2024
@mf2vec-dev mf2vec-dev deleted the fix-segfault-py-del-c branch January 29, 2024 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iorsh iorsh iorsh left review comments

@skef skef skef approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mf2vec-dev @iorsh @skef