Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Segfault fix in freetype.get_version#3567

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
MyreMylar merged 6 commits intopygame:mainfromoddbookworm:freetype_segfault
Dec 16, 2022

Conversation

@oddbookworm
Copy link
Contributor

Fixed the segfault reported in#3564

@Matiiss
Copy link
Contributor

I don't know about this, until now it could show the version even if it wasn't initialized which kind of makes sense, so following that this should also just show the current version instead of raising an exception. However, this makes sense too, it just doesn't conform to the previous versions.

@oddbookworm
Copy link
ContributorAuthor

@Matiiss here is what_ft_get_version looked like before#3379:

static PyObject *_ft_get_version(PyObject *self, PyObject *_null){    /* Return the linked FreeType2 version */    return Py_BuildValue("iii", FREETYPE_MAJOR, FREETYPE_MINOR,                         FREETYPE_PATCH);}

Although, I'm pretty sure that comment is wrong and those macros are the compiled version. That code still exists after the PR merge from above, but the linked version requires aFreeTypeInstance, and the one initialized in pygame doesn't get filled untilpygame.freetype.init() is called. So, the compiled version can be returned without raising an exception, but the linked version cannot currently.

@oddbookworm
Copy link
ContributorAuthor

oddbookworm commentedNov 15, 2022
edited
Loading

In fact, here's the declaration of those macros in thefreetype.h header in the FreeType2 lib files:

  /**************************************************************************   *   * @enum:   *   FREETYPE_XXX   *   * @description:   *   These three macros identify the FreeType source code version.  Use   *   @FT_Library_Version to access them at runtime.   *   * @values:   *   FREETYPE_MAJOR ::   *     The major version number.   *   FREETYPE_MINOR ::   *     The minor version number.   *   FREETYPE_PATCH ::   *     The patch level.   *   * @note:   *   The version number of FreeType if built as a dynamic link library with   *   the 'libtool' package is _not_ controlled by these three macros.   *   */#define FREETYPE_MAJOR  2#define FREETYPE_MINOR  11#define FREETYPE_PATCH  1

@oddbookworm
Copy link
ContributorAuthor

But I think I have actually found a solution, so I'll add another commit

Comment on lines 2055 to 2056
PyErr_SetString(pgExc_SDLError,
"FreeType could not be initialized");
Copy link
Contributor

Choose a reason for hiding this comment

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

how aboutPyExc_RuntimeError ?

@MyreMylar
Copy link
Contributor

MyreMylar commentedNov 19, 2022
edited
Loading

Would it not be better just to do:

if (linked) {    if (!inst) {            PyErr_SetString(PyExc_RuntimeError,                            "FreeType is not be initialized");        return NULL;    }

Right now it seems like this would be leaking memory from initialising two versions of freetype and the one inget_version() not being cleaned up?

Starbuck5 reacted with thumbs up emoji

@oddbookworm
Copy link
ContributorAuthor

@MyreMylar I see two routes:

  1. The way I have done it (with fixes to make sure freetype gets cleaned up properly inget_version)
  2. Ifpygame.freetype is not initialized, either throw an error or return something like(0, 0, 0)

With option 1, I understand that it's not the most ideal solution because of the extra initialization of freetype. But, I also would like to see this method behave similarly to the other getters, who don't need module initialization within pygame to work. Which is why I opted to pursue that route (and then forgetting to clean up after myself). Also considering that this method is intended to help diagnose problems with pygame, I think it would be bad form to throw an error or return a dummy value just because a module isn't initialized.

I'd like to hear your thoughts about it. In the meantime, I'll go ahead and push a commit hopefully cleaning up properly (for some reason, FreeType2's docs are really hard to read for me)

Starbuck5 reacted with thumbs up emoji

@Starbuck5
Copy link
Contributor

Thoughts--

  • This only broke because of the linked/compiled change to the get_version() function.
  • I agree with Myre it would be simpler just to error, but I like Andrew's thoughtfulness to want to make it behave like everything else, and it doesn't seem too complex to do so.
  • Why the changes to ft_wrap.c / ft_wrap.h?

@oddbookworm
Copy link
ContributorAuthor

Why the changes to ft_wrap.c / ft_wrap.h?

It’s more of a “returning those lines back to the way they were before the PR that broke this function in the first place

@Starbuck5
Copy link
Contributor

Should we add a test that freetype.get_version() can be called before pygame.freetype.init?

Copy link
Contributor

@Starbuck5Starbuck5 left a comment

Choose a reason for hiding this comment

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

Good stuff, approved.

Copy link
Contributor

@MyreMylarMyreMylar left a comment

Choose a reason for hiding this comment

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

LGTM 👍

illume pushed a commit that referenced this pull requestApr 30, 2023
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

2 more reviewers

@MyreMylarMyreMylarMyreMylar approved these changes

@Starbuck5Starbuck5Starbuck5 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

freetypepygame.freetypesegfault

Projects

None yet

Milestone

2.2.0

Development

Successfully merging this pull request may close these issues.

4 participants

@oddbookworm@Matiiss@MyreMylar@Starbuck5

[8]ページ先頭

©2009-2025 Movatter.jp