← 返回首页
bpo-1635741: Fix refleaks of timemoudle in PyInit_time() by shihai1991 · Pull Request #18486 · python/cpython · GitHub
Skip to content

Navigation Menu

Toggle navigation
Sign in
Appearance settings
Search or jump to...

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

Provide feedback

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

Include my email address so I can be contacted
Cancel Submit feedback

Saved searches

Use saved searches to filter your results more quickly

Cancel Create saved search
Appearance settings
Resetting focus

bpo-1635741: Fix refleaks of timemoudle in PyInit_time() #18486

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.

Sign up for GitHub

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

Merged
vstinner merged 2 commits into from
Mar 11, 2020
Conversation 11 Commits 2 Checks 0 Files changed
Merged

bpo-1635741: Fix refleaks of timemoudle in PyInit_time() #18486

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1704c00
Fix refleaks of timemoudle in PyInit_time()
shihai1991 Feb 12, 2020
54317ff
remove redundnat PyErr_Occured() in PyInit_time()
shihai1991 Feb 14, 2020
File filter

Filter by extension

Filter by extension .c  (1) All 1 file type selected Viewed files
Conversations
Failed to load comments. Retry
Loading
Jump to
Jump to file
Failed to load files. Retry
Loading
Diff view
Unified
Split
Hide whitespace
Apply and reload
Show whitespace
Diff view
Unified
Split
Hide whitespace
Apply and reload
63 changes: 45 additions & 18 deletions Modules/timemodule.c
Show comments
View file Edit file Delete file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Original file line number Diff line number Diff line change
Expand Up @@ -1759,52 +1759,78 @@ PyInit_time(void)

/* Set, or reset, module variables like time.timezone */
if (init_timezone(m) < 0) {
return NULL;
goto error;
}

#if defined(HAVE_CLOCK_GETTIME) || defined(HAVE_CLOCK_SETTIME) || defined(HAVE_CLOCK_GETRES)

#ifdef CLOCK_REALTIME
PyModule_AddIntMacro(m, CLOCK_REALTIME);
if (PyModule_AddIntMacro(m, CLOCK_REALTIME) < 0) {

pganssle Feb 12, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

Is the reason that you need goto in every one of these blocks that PyErr_Occcurred() will get reset if you make other PyModule_AddIntMacro calls that don't fail along the way?

Either way, I'm mildly tempted to suggest adding a wrapper macro like:

#define _PyModule_AddIntMacroOrGotoError(m, macro) \ if (PyModule_AddIntMacro(m, macro) < 0) { goto error; }

But I dunno. I'm torn between the principles of "don't repeat yourself" and "don't write a preprocessor macro with a goto in it (you monster)".

brandtbucher Feb 12, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

Between the two options, I'm personally +0 on repetition. Incorrect error handling for the whole PyModule_Add family of functions is rampant in the stdlib (see my work in bpo-38823), and I think reinforcing the correct usage is good to have to keep people from repeating the same old patterns (especially in outside projects).

shihai1991 Feb 13, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

Is the reason that you need goto in every one of these blocks that PyErr_Occcurred() will get reset if you make other PyModule_AddIntMacro calls that don't fail along the way?

No, I just try to make the Py_DECREF(m);return NULL; looks like a common processing item.

I think reinforcing the correct usage is good to have to keep people from repeating the same old patterns

+1

vstinner Mar 11, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

I dislike macros hiding a goto. A goto statement is dangerous, I prefer to make it explicit. The "if (...) { goto error; }" is a very common pattern in Python, especially in initialization code.
goto error;
}
#endif
#ifdef CLOCK_MONOTONIC
PyModule_AddIntMacro(m, CLOCK_MONOTONIC);
if (PyModule_AddIntMacro(m, CLOCK_MONOTONIC) < 0) {
goto error;
}
#endif
#ifdef CLOCK_MONOTONIC_RAW
PyModule_AddIntMacro(m, CLOCK_MONOTONIC_RAW);
if (PyModule_AddIntMacro(m, CLOCK_MONOTONIC_RAW) < 0) {
goto error;
}
#endif
#ifdef CLOCK_HIGHRES
PyModule_AddIntMacro(m, CLOCK_HIGHRES);
if (PyModule_AddIntMacro(m, CLOCK_HIGHRES) < 0) {
goto error;
}
#endif
#ifdef CLOCK_PROCESS_CPUTIME_ID
PyModule_AddIntMacro(m, CLOCK_PROCESS_CPUTIME_ID);
if (PyModule_AddIntMacro(m, CLOCK_PROCESS_CPUTIME_ID) < 0) {
goto error;
}
#endif
#ifdef CLOCK_THREAD_CPUTIME_ID
PyModule_AddIntMacro(m, CLOCK_THREAD_CPUTIME_ID);
if (PyModule_AddIntMacro(m, CLOCK_THREAD_CPUTIME_ID) < 0) {
goto error;
}
#endif
#ifdef CLOCK_PROF
PyModule_AddIntMacro(m, CLOCK_PROF);
if (PyModule_AddIntMacro(m, CLOCK_PROF) < 0) {
goto error;
}
#endif
#ifdef CLOCK_BOOTTIME
PyModule_AddIntMacro(m, CLOCK_BOOTTIME);
if (PyModule_AddIntMacro(m, CLOCK_BOOTTIME) < 0) {
goto error;
}
#endif
#ifdef CLOCK_UPTIME
PyModule_AddIntMacro(m, CLOCK_UPTIME);
if (PyModule_AddIntMacro(m, CLOCK_UPTIME) < 0) {
goto error;
}
#endif
#ifdef CLOCK_UPTIME_RAW
PyModule_AddIntMacro(m, CLOCK_UPTIME_RAW);
if (PyModule_AddIntMacro(m, CLOCK_UPTIME_RAW) < 0) {
goto error;
}
#endif

#endif /* defined(HAVE_CLOCK_GETTIME) || defined(HAVE_CLOCK_SETTIME) || defined(HAVE_CLOCK_GETRES) */

if (!initialized) {
if (PyStructSequence_InitType2(&StructTimeType,
&struct_time_type_desc) < 0)
return NULL;
&struct_time_type_desc) < 0) {
goto error;
}
}
if (PyModule_AddIntConstant(m, "_STRUCT_TM_ITEMS", 11)) {
goto error;
}
Py_INCREF(&StructTimeType);
PyModule_AddIntConstant(m, "_STRUCT_TM_ITEMS", 11);
PyModule_AddObject(m, "struct_time", (PyObject*) &StructTimeType);
if (PyModule_AddObject(m, "struct_time", (PyObject*) &StructTimeType)) {
Py_DECREF(&StructTimeType);
goto error;
}
initialized = 1;

#if defined(__linux__) && !defined(__GLIBC__)
Expand All @@ -1814,10 +1840,11 @@ PyInit_time(void)
utc_string = tm.tm_zone;
#endif

if (PyErr_Occurred()) {
return NULL;
}
return m;

error:
Py_DECREF(m);
return NULL;
}

/* Implement pysleep() for various platforms.
Expand Down
Toggle all file notes Toggle all file annotations

Footer

© 2026 GitHub, Inc.