gh-123290: curses: fix reference leaks in error-branches and cleanup module's initialization #123910

picnixz · 2024-09-10T15:01:06Z

In this PR, we prepare the cursesmodule for #123630 and clean-up some bits. To that end, we:

ensure that error branches correctly free resources (even if the error is usually "critical", in the sense that the interpreter would exit after reporting it).
protect macros against dangling else using do { ... } while (0) constructions.
use upper-case names for global variables; this helps the readability of the module.

There are some un-necessary PyObject * casts that will be removed if I were to move from a static type to a heap type, so I won't do it now.

Now I have an important question. There is a global variable named ModDict which is set to the module's dict upon initialization. It is then used in initscr() to add other constants. However, AFACIT, this variable is only used as a convenience to avoid re-querying the module's dict, right? Or is there some deeper meaning where I must use a global variable to store tha module's dict?

cc @vstinner @encukou

Issue: Multiple reference leaks in curses error-branches #123290

vstinner · 2024-09-10T16:07:51Z

Modules/_cursesmodule.c

-    if (wo->encoding != NULL)
+    if (wo->win != stdscr && wo->win != NULL) {
+        delwin(wo->win);
+        wo->win = NULL;


I don't think that it's useful since we free() the memory a few lines below. Same remark for wo->encoding = NULL;.

I didn't really know whether delwin(wo->win) checks if its argument is NULL or not so I wanted to be sure (I'll check the manual more in detail tomorrow).

FTR, from the man page:

X/Open defines no error conditions. In this implementation

delwin
returns an error if the window pointer is null, or if the window is the parent of another window.
This implementation also maintains a list of windows, and checks that the pointer passed to delwin is one that it created, returning an error if it was not.

I added a comment saying that we silently ignore any error in delwin.

vstinner · 2024-09-10T16:08:14Z

Modules/_cursesmodule.c

+        delwin(wo->win);
+        wo->win = NULL;
+    }
+    if (wo->encoding != NULL) {


This check is not needed, PyMem_Free(NULL) is well defined: it does nothing.

Ah, I see why I thought it wasn't. Actually, the online docs tell me that it's a no-op but the implementation does not tell me directly. I'll make the modifications tomorrow as well (actually I'll do everything tomorrow).

PyMem_Free():

If p is NULL, no operation is performed.

https://docs.python.org/dev/c-api/memory.html#c.PyMem_Free

vstinner · 2024-09-10T16:13:06Z

Modules/_cursesmodule.c

-        return NULL;
-    }
+    PyObject *result = PyCursesCheckERR(resize_term(nlines, ncols), "resize_term");
+    CHECK_NOT_NULL_OR_ERROR(result);


I dislike CHECK_NOT_NULL_OR_ERROR() macro, it makes the code less readable. I prefer the explicit existing if (!result) return NULL; code.

vstinner · 2024-09-10T16:13:56Z

Modules/_cursesmodule.c

-    }
+    PyObject *result = PyCursesCheckERR(resize_term(nlines, ncols), "resize_term");
+    CHECK_NOT_NULL_OR_ERROR(result);
+    CHECK_RET_FLAG_OR_ERROR(update_lines_cols(module));


I don't understand this macro. I would prefer:

if (!update_lines_cols(module)) goto error;

CHECK_RET_FLAG_OR_ERROR + goto makes the code less straightforward.

Maybe my naming sense is terrible but it was something I found in the code sometimes (e.g., CHECK_INT in _decimal.c). But I'll change the macros since it's probably better to reduce the diff rather than reducing the overall number of lines.

EDIT: I think I know why the macro was confusing (probably because of the FLAG part; should have been CHECK_RET_BOOL but it's not really a good idea either). I'll remove it and use an explicit if (!(...)).

vstinner · 2024-09-10T16:15:54Z

Modules/_cursesmodule.c


-static char *screen_encoding = NULL;
+static char *CURSES_SCREEN_ENCODING = NULL;


Are these 4 variables moved to the module state later? If yes, these changes look redundant. If no, why not? :-)

Are these 4 variables moved to the module state later?

No. Because I totally forgot about them in the other PR. But here, when I was isolating the components to select, I was like "hum, but those are actually global variables" and it confused me to have them in lowercase as if they were local variables :(

I think that i'm ok with renaming these variables, but I'm less comfortable with uppercase names. Uppercase is somehow reserved to macros in C (in CPython at least). I suggest to rename these variables to lowercase.

vstinner · 2024-09-10T16:17:06Z

Modules/_cursesmodule.c

-    }
+    CHECK_NOT_NULL_OR_ERROR(o);
+    CHECK_RET_CODE_OR_ERROR(PyObject_SetAttrString(exposed_module, "LINES", o));
+    CHECK_RET_CODE_OR_ERROR(PyObject_SetAttrString(private_module, "LINES", o));


I would expect a macro getting a module, a key and a value. Rather than a macro taking a function call.

vstinner · 2024-09-10T16:19:35Z

Modules/_cursesmodule.c

+        }                               \
+    } while (0)
+
+#define DICT_ADD_INT_VALUE_OR_ERROR(DICT_OBJECT, STRING, VALUE)         \


I suggest to rename the macro to DICT_ADD_INT_VALUE.

I thought about it but wondered whether it was necessary to indicate in the name the fact that we jump to the error label. If you think a comment is sufficient, I can rename it.

vstinner · 2024-09-10T16:20:13Z

Modules/_cursesmodule.c

+        if (!(STATUS)) {                \
+            goto error;                 \
+        }                               \
+    } while (0)


I don't think that these 3 macros are useful. They hide the goto which makes it to follow code using them (IMO).

I think it's a matter of personal taste but I can also understand your stand on that matter. Personally, what I find hard to read is when you have all those if(...) { goto error; } spread on 3 lines. I would find them much easier to read if PEP 7 allowed me to write if (!(STATUS)) goto error;. On the other hand, with those macros, you may be also introduce errors where you need to free resources before jumping to error so they should be used carefully...

I had alternatives where you would just put whatever cleanup statements you want when invoking the macro but I felt it would be even less readable...

picnixz · 2024-09-10T16:40:52Z

Thanks for the review Victor. I'll address your comments tomorrow!

picnixz · 2024-09-11T11:09:35Z

@vstinner I eventually removed the if (x == NULL) goto error and the if (!x) goto error macros. I kept the CHECK_RET_CODE_OR_ERROR and the macro for adding an int to a dict but removed their _OR_ERROR suffixes and wrote a comment instead.

I also added a local macro for adding the LINES and COLS constants. Please tell me if there is anything else to change! (again, thanks a lot for your feedback).

vstinner

I have mixed feelings about this PR. IMO it changes too many different things at once, so it's hard to review. For example, it adds a module argument to update_lines_cols(): the rationale for this change is unclear to me. It also adds ; to PyCursesInitialised which is unrelated. It also renames some variables. And the main part: fixing reference leaks.

Would it be possible to try to write a minimum change only to fix refleaks? Without the large refactoring?

vstinner · 2024-09-11T12:19:00Z

Modules/_cursesmodule.c

+#define CHECK_STATE_FLAG(FUNC_TO_CALL, GLOBAL_FLAG)                 \
+    do {                                                            \
+        if ((GLOBAL_FLAG) != TRUE) {                                \
+            assert(PyCursesError != NULL);                          \


I would suggest to remove this assertion. It sounds unlikely that the exception is NULL when this macro is used. If there is a risk, I would suggest to handle the test in the caller instead.

vstinner · 2024-09-11T12:21:48Z

Modules/_cursesmodule.c

-    Py_DECREF(m);
+#define MODULE_ADD_INT_CONSTANT(MODULE, NAME, VALUE)                        \
+    do {                                                                    \
+        if (PyModule_AddIntConstant((MODULE), (NAME), (long)(VALUE)) < 0) { \


Is this (long) cast really needed?

I think the compiler won't complain but I'm not entirely sure since we use macros. If an implementation uses typed global variables, we might want to force the cast (I've seen both usages of explicit casting and sometimes not but I don't think the cast hurts)

vstinner · 2024-09-11T12:22:55Z

Modules/_cursesmodule.c

-            Py_DECREF(c);
+    if (start_color() != ERR) {
+        CURSES_INITIALISED_COLORS = TRUE;
+        PyObject *module_dict = PyModule_GetDict(module);


Suggested change

PyObject *module_dict = PyModule_GetDict(module);

PyObject *module_dict = PyModule_GetDict(module); // borrowed ref

vstinner · 2024-09-11T12:23:39Z

Modules/_cursesmodule.c

-            return NULL;
-        if (PyDict_SetItemString(ModDict, "COLORS", c) < 0) {
-            Py_DECREF(c);
+    if (start_color() != ERR) {


Can you exit early? Start with if (start_color() == ERR) { ... return ... } so you can remove one indentation level.

vstinner · 2024-09-11T12:27:14Z

Modules/_cursesmodule.c


-static char *screen_encoding = NULL;
+static char *CURSES_SCREEN_ENCODING = NULL;


I think that i'm ok with renaming these variables, but I'm less comfortable with uppercase names. Uppercase is somehow reserved to macros in C (in CPython at least). I suggest to rename these variables to lowercase.

vstinner · 2024-09-11T12:30:13Z

Modules/_cursesmodule.c

@@ -3967,49 +4010,32 @@ _curses_qiflush_impl(PyObject *module, int flag)
 /* Internal helper used for updating curses.LINES, curses.COLS, _curses.LINES
 * and _curses.COLS */
 #if defined(HAVE_CURSES_RESIZETERM) || defined(HAVE_CURSES_RESIZE_TERM)
-static int
-update_lines_cols(void)
+static int /* 1 on success, 0 on failure */


Can you move this comment in the comment 1 line above?

vstinner · 2024-09-11T12:35:09Z

Modules/_cursesmodule.c

+
+#undef DICT_ADD_INT_VALUE
+#undef CHECK_RET_CODE
+#undef CHECK_STATE_FLAG


I don't think that it's useful to undefine macros at the end of the file. It's ok to have macros defined for a whole C file.

picnixz · 2024-09-11T14:35:02Z

I discussed with Victor and I'll split the work (again) into smaller PRs:

PR fixing ref-leaks in error branches
PR protecting existing macros using do { ... } while (0) constructions
PR renaming global variables and removing the ModDict global variable as well.

(Hence, I'm closing this one, again).

picnixz added 7 commits September 10, 2024 16:24

Fix reference leaks in the error-branches.

3ce06f4

protect macros expansions using do { ... } while (0) constructions

81bcde8

use uppercase global flags

848cc3e

cosmetic changes

ea3fd8d

improve SetDictInt macro

557a53c

undef utililty macros

a92fc1f

improve macro usage

9ce0289

picnixz added the skip news label Sep 10, 2024

bedevere-app bot added the awaiting review label Sep 10, 2024

picnixz changed the title ~~gh-123630: curses: fix reference leaks in error-branches and cleanup module's initialization~~ gh-123290: curses: fix reference leaks in error-branches and cleanup module's initialization Sep 10, 2024

picnixz mentioned this pull request Sep 10, 2024

Multiple reference leaks in curses error-branches #123290

Closed

update globals

fd7372a

picnixz requested a review from ericsnowcurrently as a code owner September 10, 2024 16:05

picnixz removed the request for review from ericsnowcurrently September 10, 2024 16:07

vstinner reviewed Sep 10, 2024

View reviewed changes

picnixz mentioned this pull request Sep 10, 2024

gh-123913: Fix NULL handling in _curses_initscr_impl of _cursesmodule #123914

Closed

sobolevn mentioned this pull request Sep 10, 2024

cursesmodule uses winobj which might be NULL #123913

Closed

address Victor's review

384716d

picnixz force-pushed the curses/consistent-cleanup-123630 branch from 31cbba0 to 384716d Compare September 11, 2024 10:54

picnixz added 3 commits September 11, 2024 12:58

fix compilation if Py_GIL_DISABLED is defined

24f47a1

use a local macro

4a1003c

remove unused undefs

356b00c

picnixz force-pushed the curses/consistent-cleanup-123630 branch from 32996a1 to 356b00c Compare September 11, 2024 11:07

vstinner reviewed Sep 11, 2024

View reviewed changes

picnixz closed this Sep 11, 2024

picnixz deleted the curses/consistent-cleanup-123630 branch September 11, 2024 14:55

picnixz mentioned this pull request Sep 13, 2024

Add curses prefix to global variables names in _cursesmodule.c #124046

Closed

picnixz mentioned this pull request Sep 13, 2024

Refactorization of _cursesmodule.c to fix reference leaks #123961

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-123290: curses: fix reference leaks in error-branches and cleanup module's initialization #123910

gh-123290: curses: fix reference leaks in error-branches and cleanup module's initialization #123910

picnixz commented Sep 10, 2024 •

edited by bedevere-app bot

Loading

vstinner Sep 10, 2024

picnixz Sep 10, 2024

picnixz Sep 11, 2024

vstinner Sep 10, 2024

picnixz Sep 10, 2024

vstinner Sep 11, 2024

vstinner Sep 10, 2024

vstinner Sep 10, 2024

picnixz Sep 10, 2024 •

edited

Loading

picnixz Sep 10, 2024 •

edited

Loading

vstinner Sep 10, 2024

picnixz Sep 10, 2024 •

edited

Loading

vstinner Sep 11, 2024

vstinner Sep 10, 2024

vstinner Sep 10, 2024

picnixz Sep 10, 2024

vstinner Sep 10, 2024

picnixz Sep 10, 2024

picnixz commented Sep 10, 2024 •

edited

Loading

picnixz commented Sep 11, 2024

vstinner left a comment

vstinner Sep 11, 2024

vstinner Sep 11, 2024

picnixz Sep 11, 2024

vstinner Sep 11, 2024

vstinner Sep 11, 2024

vstinner Sep 11, 2024

vstinner Sep 11, 2024

vstinner Sep 11, 2024

picnixz commented Sep 11, 2024 •

edited

Loading


		static char *screen_encoding = NULL;
		static char *CURSES_SCREEN_ENCODING = NULL;

	PyObject *module_dict = PyModule_GetDict(module);
	PyObject *module_dict = PyModule_GetDict(module); // borrowed ref

gh-123290: curses: fix reference leaks in error-branches and cleanup module's initialization #123910

gh-123290: curses: fix reference leaks in error-branches and cleanup module's initialization #123910

Conversation

picnixz commented Sep 10, 2024 • edited by bedevere-app bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

picnixz Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

picnixz Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

picnixz Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

picnixz commented Sep 10, 2024 • edited Loading

picnixz commented Sep 11, 2024

vstinner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

picnixz commented Sep 11, 2024 • edited Loading

picnixz commented Sep 10, 2024 •

edited by bedevere-app bot

Loading

picnixz Sep 10, 2024 •

edited

Loading

picnixz Sep 10, 2024 •

edited

Loading

picnixz Sep 10, 2024 •

edited

Loading

picnixz commented Sep 10, 2024 •

edited

Loading

picnixz commented Sep 11, 2024 •

edited

Loading