Skip to content

bpo-43687: use unicode_state empty string before unicode_init. without define WITH_DOC_STRINGS #25129

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

Closed
wants to merge 1 commit into from

Conversation

JunyiXie
Copy link
Contributor

@JunyiXie JunyiXie commented Apr 1, 2021

without define WITH_DOC_STRINGS. use unicode_state empty string before _PyUnicode_Init.

PyType_Ready call PyUnicode_FromString, if doc string "", cause crash.

unicode_get_empty() must not be called before _PyUnicode_Init() or after _PyUnicode_Fini()

PyType_Ready

const char *old_doc = _PyType_DocWithoutSignature(type->tp_name,type->tp_doc);
PyObject *doc = PyUnicode_FromString(old_doc);

https://bugs.python.org/issue43687

@JunyiXie
Copy link
Contributor Author

JunyiXie commented Apr 1, 2021

This commit should not need to add news.

@JunyiXie
Copy link
Contributor Author

JunyiXie commented Apr 2, 2021

@vstinner Please review this pr? thanks.I found this crash, I am not sure that my fix are reasonable.

PyObject *empty = unicode_get_empty();
Py_INCREF(empty);
PyObject *empty = NULL;
if (_Py_unicode_inited == 0) {
Copy link
Member

Choose a reason for hiding this comment

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

This change removes my micro-optimization:

static int
unicode_create_empty_string_singleton(struct _Py_unicode_state *state)
{
    // Use size=1 rather than size=0, so PyUnicode_New(0, maxchar) can be
    // optimized to always use state->empty_string without having to check if
    // it is NULL or not.

I wrote PR #25147 which reorganizes Py_Initialize() to fix https://bugs.python.org/issue43687 without losing the optimization.

@vstinner
Copy link
Member

vstinner commented Apr 2, 2021

I merged PR #25147 instead, but thanks anyway.

@vstinner vstinner closed this Apr 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants