Skip to content

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
merged 2 commits into from
Mar 11, 2020
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
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 45 additions & 18 deletions Modules/timemodule.c
View file
Open in desktop
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) {
Copy link
Member

@pganssle pganssle Feb 12, 2020

Choose a reason for hiding this comment

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

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)".

Copy link
Member

@brandtbucher brandtbucher Feb 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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).

Copy link
Member Author

@shihai1991 shihai1991 Feb 13, 2020

Choose a reason for hiding this comment

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

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

Copy link
Member

@vstinner vstinner Mar 11, 2020

Choose a reason for hiding this comment

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

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