From b6a0e1cad42c51dd42a7b0278ce0330303c6e114 Mon Sep 17 00:00:00 2001 From: Irit Katriel Date: Fri, 3 Dec 2021 11:46:30 +0000 Subject: [PATCH 1/6] bpo-45635: standardize error handling in traceback.c --- Python/traceback.c | 139 ++++++++++++++++++++++++++++----------------- 1 file changed, 88 insertions(+), 51 deletions(-) diff --git a/Python/traceback.c b/Python/traceback.c index 8aef3d810d3162..6384cae4cb0019 100644 --- a/Python/traceback.c +++ b/Python/traceback.c @@ -393,12 +393,12 @@ _Py_WriteIndent(int indent, PyObject *f) buf[indent] = '\0'; } err = PyFile_WriteString(buf, f); - if (err != 0) { - return err; - } + if (err == -1) + goto done; indent -= 10; } - return 0; +done: + return err; } /* Writes indent spaces, followed by the margin if it is not `\0`. @@ -408,9 +408,14 @@ int _Py_WriteIndentedMargin(int indent, const char *margin, PyObject *f) { int err = _Py_WriteIndent(indent, f); - if (err == 0 && margin) { + if (err == -1) + goto done; + if (margin) { err = PyFile_WriteString(margin, f); + if (err == -1) + goto done; } +done: return err; } @@ -545,22 +550,28 @@ display_source_line_with_margin(PyObject *f, PyObject *filename, int lineno, int *truncation = i - indent; } - if (err == 0) { - err = _Py_WriteIndentedMargin(margin_indent, margin, f); - } + assert(!err); + assert(lineobj); + + err = _Py_WriteIndentedMargin(margin_indent, margin, f); + if (err == -1) + goto done; + /* Write some spaces before the line */ - if (err == 0) { - err = _Py_WriteIndent(indent, f); - } + err = _Py_WriteIndent(indent, f); + if (err == -1) + goto done; /* finally display the line */ - if (err == 0) { - err = PyFile_WriteObject(lineobj, f, Py_PRINT_RAW); - } + err = PyFile_WriteObject(lineobj, f, Py_PRINT_RAW); + if (err == -1) + goto done; + + err = PyFile_WriteString("\n", f); + if (err == -1) + goto done; +done: Py_DECREF(lineobj); - if (err == 0) { - err = PyFile_WriteString("\n", f); - } return err; } @@ -725,16 +736,24 @@ print_error_location_carets(PyObject *f, int offset, Py_ssize_t start_offset, Py const char *primary, const char *secondary) { int err = 0; int special_chars = (left_end_offset != -1 || right_start_offset != -1); + const char *str; while (++offset <= end_offset) { if (offset <= start_offset || offset > end_offset) { - err = PyFile_WriteString(" ", f); + str = " "; } else if (special_chars && left_end_offset < offset && offset <= right_start_offset) { - err = PyFile_WriteString(secondary, f); + str = secondary; } else { - err = PyFile_WriteString(primary, f); + str = primary; } + err = PyFile_WriteString(str, f); + if (err == -1) + goto done; } err = PyFile_WriteString("\n", f); + if (err == -1) + goto done; + +done: return err; } @@ -742,22 +761,22 @@ static int tb_displayline(PyTracebackObject* tb, PyObject *f, PyObject *filename, int lineno, PyFrameObject *frame, PyObject *name, int margin_indent, const char *margin) { - int err; - PyObject *line; - if (filename == NULL || name == NULL) return -1; - line = PyUnicode_FromFormat(" File \"%U\", line %d, in %U\n", - filename, lineno, name); + + int err = _Py_WriteIndentedMargin(margin_indent, margin, f); + if (err == -1) + return -1; + + PyObject *line = PyUnicode_FromFormat(" File \"%U\", line %d, in %U\n", + filename, lineno, name); if (line == NULL) return -1; - err = _Py_WriteIndentedMargin(margin_indent, margin, f); - if (err == 0) { - err = PyFile_WriteObject(line, f, Py_PRINT_RAW); - } + + err = PyFile_WriteObject(line, f, Py_PRINT_RAW); Py_DECREF(line); - if (err != 0) - return err; + if (err == -1) + return -1; int truncation = _TRACEBACK_SOURCE_LINE_INDENT; PyObject* source_line = NULL; @@ -850,11 +869,14 @@ tb_displayline(PyTracebackObject* tb, PyObject *f, PyObject *filename, int linen } err = _Py_WriteIndentedMargin(margin_indent, margin, f); - if (err == 0) { - err = print_error_location_carets(f, truncation, start_offset, end_offset, - right_start_offset, left_end_offset, - primary_error_char, secondary_error_char); - } + if (err == -1) + goto done; + + err = print_error_location_carets(f, truncation, start_offset, end_offset, + right_start_offset, left_end_offset, + primary_error_char, secondary_error_char); + if (err == -1) + goto done; done: Py_XDECREF(source_line); @@ -884,6 +906,7 @@ static int tb_printinternal(PyTracebackObject *tb, PyObject *f, long limit, int indent, const char *margin) { + PyCodeObject *code = NULL; int err = 0; Py_ssize_t depth = 0; PyObject *last_file = NULL; @@ -900,13 +923,15 @@ tb_printinternal(PyTracebackObject *tb, PyObject *f, long limit, tb = tb->tb_next; } while (tb != NULL && err == 0) { - PyCodeObject *code = PyFrame_GetCode(tb->tb_frame); + code = PyFrame_GetCode(tb->tb_frame); if (last_file == NULL || code->co_filename != last_file || last_line == -1 || tb->tb_lineno != last_line || last_name == NULL || code->co_name != last_name) { if (cnt > TB_RECURSIVE_CUTOFF) { err = tb_print_line_repeated(f, cnt); + if (err == -1) + goto done; } last_file = code->co_filename; last_line = tb->tb_lineno; @@ -914,19 +939,26 @@ tb_printinternal(PyTracebackObject *tb, PyObject *f, long limit, cnt = 0; } cnt++; - if (err == 0 && cnt <= TB_RECURSIVE_CUTOFF) { + if (cnt <= TB_RECURSIVE_CUTOFF) { err = tb_displayline(tb, f, code->co_filename, tb->tb_lineno, tb->tb_frame, code->co_name, indent, margin); - if (err == 0) { - err = PyErr_CheckSignals(); - } + if (err == -1) + goto done; + + err = PyErr_CheckSignals(); + if (err == -1) + goto done; } - Py_DECREF(code); + Py_CLEAR(code); tb = tb->tb_next; } - if (err == 0 && cnt > TB_RECURSIVE_CUTOFF) { + if (cnt > TB_RECURSIVE_CUTOFF) { err = tb_print_line_repeated(f, cnt); + if (err == -1) + goto done; } +done: + Py_XDECREF(code); return err; } @@ -936,7 +968,6 @@ int _PyTraceBack_Print_Indented(PyObject *v, int indent, const char *margin, const char *header_margin, const char *header, PyObject *f) { - int err; PyObject *limitv; long limit = PyTraceBack_LIMIT; @@ -957,13 +988,19 @@ _PyTraceBack_Print_Indented(PyObject *v, int indent, const char *margin, return 0; } } - err = _Py_WriteIndentedMargin(indent, header_margin, f); - if (err == 0) { - err = PyFile_WriteString(header, f); - } - if (err == 0) { - err = tb_printinternal((PyTracebackObject *)v, f, limit, indent, margin); - } + int err = _Py_WriteIndentedMargin(indent, header_margin, f); + if (err == -1) + goto done; + + err = PyFile_WriteString(header, f); + if (err == -1) + goto done; + + err = tb_printinternal((PyTracebackObject *)v, f, limit, indent, margin); + if (err == -1) + goto done; + +done: return err; } From afca4c7e7f5d89f98e34a3a0360c1bc7e35e011c Mon Sep 17 00:00:00 2001 From: Irit Katriel Date: Fri, 3 Dec 2021 22:40:49 +0000 Subject: [PATCH 2/6] Erlend's style suggestions --- Python/traceback.c | 180 ++++++++++++++++++++++++--------------------- 1 file changed, 97 insertions(+), 83 deletions(-) diff --git a/Python/traceback.c b/Python/traceback.c index 6384cae4cb0019..edb9073ca4603d 100644 --- a/Python/traceback.c +++ b/Python/traceback.c @@ -385,20 +385,20 @@ _Py_FindSourceFile(PyObject *filename, char* namebuf, size_t namelen, PyObject * int _Py_WriteIndent(int indent, PyObject *f) { - int err = 0; char buf[11] = " "; assert(strlen(buf) == 10); while (indent > 0) { if (indent < 10) { buf[indent] = '\0'; } - err = PyFile_WriteString(buf, f); - if (err == -1) - goto done; + if (PyFile_WriteString(buf, f) < 0) { + goto error; + } indent -= 10; } -done: - return err; + return 0; +error: + return -1; } /* Writes indent spaces, followed by the margin if it is not `\0`. @@ -407,16 +407,17 @@ _Py_WriteIndent(int indent, PyObject *f) int _Py_WriteIndentedMargin(int indent, const char *margin, PyObject *f) { - int err = _Py_WriteIndent(indent, f); - if (err == -1) - goto done; + if (_Py_WriteIndent(indent, f) < 0) { + goto error; + } if (margin) { - err = PyFile_WriteString(margin, f); - if (err == -1) - goto done; + if (PyFile_WriteString(margin, f) < 0) { + goto error; + } } -done: - return err; + return 0; +error: + return -1; } static int @@ -512,10 +513,12 @@ display_source_line_with_margin(PyObject *f, PyObject *filename, int lineno, int } } res = _PyObject_CallMethodIdNoArgs(fob, &PyId_close); - if (res) + if (res) { Py_DECREF(res); - else + } + else { PyErr_Clear(); + } Py_DECREF(fob); if (!lineobj || !PyUnicode_Check(lineobj)) { Py_XDECREF(lineobj); @@ -553,26 +556,29 @@ display_source_line_with_margin(PyObject *f, PyObject *filename, int lineno, int assert(!err); assert(lineobj); - err = _Py_WriteIndentedMargin(margin_indent, margin, f); - if (err == -1) - goto done; + if (_Py_WriteIndentedMargin(margin_indent, margin, f) < 0) { + goto error; + } /* Write some spaces before the line */ - err = _Py_WriteIndent(indent, f); - if (err == -1) - goto done; + if (_Py_WriteIndent(indent, f) < 0) { + goto error; + } /* finally display the line */ - err = PyFile_WriteObject(lineobj, f, Py_PRINT_RAW); - if (err == -1) - goto done; + if (PyFile_WriteObject(lineobj, f, Py_PRINT_RAW) < 0) { + goto error; + } + + if (PyFile_WriteString("\n", f) < 0) { + goto error; + } - err = PyFile_WriteString("\n", f); - if (err == -1) - goto done; -done: Py_DECREF(lineobj); - return err; + return 0; +error: + Py_DECREF(lineobj); + return -1; } int @@ -734,7 +740,6 @@ static inline int print_error_location_carets(PyObject *f, int offset, Py_ssize_t start_offset, Py_ssize_t end_offset, Py_ssize_t right_start_offset, Py_ssize_t left_end_offset, const char *primary, const char *secondary) { - int err = 0; int special_chars = (left_end_offset != -1 || right_start_offset != -1); const char *str; while (++offset <= end_offset) { @@ -745,38 +750,43 @@ print_error_location_carets(PyObject *f, int offset, Py_ssize_t start_offset, Py } else { str = primary; } - err = PyFile_WriteString(str, f); - if (err == -1) - goto done; + if (PyFile_WriteString(str, f) < 0) { + goto error; + } } - err = PyFile_WriteString("\n", f); - if (err == -1) - goto done; - -done: - return err; + if (PyFile_WriteString("\n", f) < 0) { + goto error; + } + return 0; +error: + return -1; } static int tb_displayline(PyTracebackObject* tb, PyObject *f, PyObject *filename, int lineno, PyFrameObject *frame, PyObject *name, int margin_indent, const char *margin) { - if (filename == NULL || name == NULL) + if (filename == NULL || name == NULL) { return -1; + } - int err = _Py_WriteIndentedMargin(margin_indent, margin, f); - if (err == -1) + if (_Py_WriteIndentedMargin(margin_indent, margin, f) < 0) { return -1; + } PyObject *line = PyUnicode_FromFormat(" File \"%U\", line %d, in %U\n", filename, lineno, name); - if (line == NULL) + if (line == NULL) { return -1; + } - err = PyFile_WriteObject(line, f, Py_PRINT_RAW); + int res = PyFile_WriteObject(line, f, Py_PRINT_RAW); Py_DECREF(line); - if (err == -1) + if (res == -1) { return -1; + } + + int err = 0; int truncation = _TRACEBACK_SOURCE_LINE_INDENT; PyObject* source_line = NULL; @@ -868,15 +878,17 @@ tb_displayline(PyTracebackObject* tb, PyObject *f, PyObject *filename, int linen end_offset = i + 1; } - err = _Py_WriteIndentedMargin(margin_indent, margin, f); - if (err == -1) + if (_Py_WriteIndentedMargin(margin_indent, margin, f) < 0) { + err = -1; goto done; + } - err = print_error_location_carets(f, truncation, start_offset, end_offset, - right_start_offset, left_end_offset, - primary_error_char, secondary_error_char); - if (err == -1) + if (print_error_location_carets(f, truncation, start_offset, end_offset, + right_start_offset, left_end_offset, + primary_error_char, secondary_error_char) < 0) { + err = -1; goto done; + } done: Py_XDECREF(source_line); @@ -907,7 +919,6 @@ tb_printinternal(PyTracebackObject *tb, PyObject *f, long limit, int indent, const char *margin) { PyCodeObject *code = NULL; - int err = 0; Py_ssize_t depth = 0; PyObject *last_file = NULL; int last_line = -1; @@ -922,16 +933,16 @@ tb_printinternal(PyTracebackObject *tb, PyObject *f, long limit, depth--; tb = tb->tb_next; } - while (tb != NULL && err == 0) { + while (tb != NULL) { code = PyFrame_GetCode(tb->tb_frame); if (last_file == NULL || code->co_filename != last_file || last_line == -1 || tb->tb_lineno != last_line || last_name == NULL || code->co_name != last_name) { if (cnt > TB_RECURSIVE_CUTOFF) { - err = tb_print_line_repeated(f, cnt); - if (err == -1) - goto done; + if (tb_print_line_repeated(f, cnt) < 0) { + goto error; + } } last_file = code->co_filename; last_line = tb->tb_lineno; @@ -940,26 +951,27 @@ tb_printinternal(PyTracebackObject *tb, PyObject *f, long limit, } cnt++; if (cnt <= TB_RECURSIVE_CUTOFF) { - err = tb_displayline(tb, f, code->co_filename, tb->tb_lineno, - tb->tb_frame, code->co_name, indent, margin); - if (err == -1) - goto done; - - err = PyErr_CheckSignals(); - if (err == -1) - goto done; + if (tb_displayline(tb, f, code->co_filename, tb->tb_lineno, + tb->tb_frame, code->co_name, indent, margin) < 0) { + goto error; + } + + if (PyErr_CheckSignals() < 0) { + goto error; + } } Py_CLEAR(code); tb = tb->tb_next; } if (cnt > TB_RECURSIVE_CUTOFF) { - err = tb_print_line_repeated(f, cnt); - if (err == -1) - goto done; + if (tb_print_line_repeated(f, cnt) < 0) { + goto error; + } } -done: + return 0; +error: Py_XDECREF(code); - return err; + return -1; } #define PyTraceBack_LIMIT 1000 @@ -971,11 +983,12 @@ _PyTraceBack_Print_Indented(PyObject *v, int indent, const char *margin, PyObject *limitv; long limit = PyTraceBack_LIMIT; - if (v == NULL) + if (v == NULL) { return 0; + } if (!PyTraceBack_Check(v)) { PyErr_BadInternalCall(); - return -1; + goto error; } limitv = PySys_GetObject("tracebacklimit"); if (limitv && PyLong_Check(limitv)) { @@ -988,20 +1001,21 @@ _PyTraceBack_Print_Indented(PyObject *v, int indent, const char *margin, return 0; } } - int err = _Py_WriteIndentedMargin(indent, header_margin, f); - if (err == -1) - goto done; + if (_Py_WriteIndentedMargin(indent, header_margin, f) < 0) { + goto error; + } - err = PyFile_WriteString(header, f); - if (err == -1) - goto done; + if (PyFile_WriteString(header, f) < 0) { + goto error; + } - err = tb_printinternal((PyTracebackObject *)v, f, limit, indent, margin); - if (err == -1) - goto done; + if (tb_printinternal((PyTracebackObject *)v, f, limit, indent, margin) < 0) { + goto error; + } -done: - return err; + return 0; +error: + return -1; } int From c18f6ea6cfac9cc6076f0da2d5cd57806091bd09 Mon Sep 17 00:00:00 2001 From: Irit Katriel Date: Sun, 5 Dec 2021 16:12:46 +0000 Subject: [PATCH 3/6] don't use gotos if there is no cleanup to do --- Python/traceback.c | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/Python/traceback.c b/Python/traceback.c index edb9073ca4603d..b5d4033627ffb2 100644 --- a/Python/traceback.c +++ b/Python/traceback.c @@ -392,13 +392,11 @@ _Py_WriteIndent(int indent, PyObject *f) buf[indent] = '\0'; } if (PyFile_WriteString(buf, f) < 0) { - goto error; + return -1; } indent -= 10; } return 0; -error: - return -1; } /* Writes indent spaces, followed by the margin if it is not `\0`. @@ -751,15 +749,13 @@ print_error_location_carets(PyObject *f, int offset, Py_ssize_t start_offset, Py str = primary; } if (PyFile_WriteString(str, f) < 0) { - goto error; + return -1; } } if (PyFile_WriteString("\n", f) < 0) { - goto error; + return -1; } return 0; -error: - return -1; } static int @@ -988,7 +984,7 @@ _PyTraceBack_Print_Indented(PyObject *v, int indent, const char *margin, } if (!PyTraceBack_Check(v)) { PyErr_BadInternalCall(); - goto error; + return -1; } limitv = PySys_GetObject("tracebacklimit"); if (limitv && PyLong_Check(limitv)) { @@ -1002,20 +998,18 @@ _PyTraceBack_Print_Indented(PyObject *v, int indent, const char *margin, } } if (_Py_WriteIndentedMargin(indent, header_margin, f) < 0) { - goto error; + return -1; } if (PyFile_WriteString(header, f) < 0) { - goto error; + return -1; } if (tb_printinternal((PyTracebackObject *)v, f, limit, indent, margin) < 0) { - goto error; + return -1; } return 0; -error: - return -1; } int From 0e25738868030b8e3b345d9c9cd84a9e75202d98 Mon Sep 17 00:00:00 2001 From: Irit Katriel <1055913+iritkatriel@users.noreply.github.com> Date: Tue, 7 Dec 2021 10:10:16 +0000 Subject: [PATCH 4/6] == -1 --> < 0 Co-authored-by: Erlend Egeberg Aasland --- Python/traceback.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Python/traceback.c b/Python/traceback.c index b5d4033627ffb2..d289cd3ffaa6f4 100644 --- a/Python/traceback.c +++ b/Python/traceback.c @@ -778,7 +778,7 @@ tb_displayline(PyTracebackObject* tb, PyObject *f, PyObject *filename, int linen int res = PyFile_WriteObject(line, f, Py_PRINT_RAW); Py_DECREF(line); - if (res == -1) { + if (res < 0) { return -1; } From 1534468ec2071aa070ac597b3978785a1934f624 Mon Sep 17 00:00:00 2001 From: Irit Katriel <1055913+iritkatriel@users.noreply.github.com> Date: Tue, 7 Dec 2021 10:10:30 +0000 Subject: [PATCH 5/6] remove unnecessary assert Co-authored-by: Erlend Egeberg Aasland --- Python/traceback.c | 1 - 1 file changed, 1 deletion(-) diff --git a/Python/traceback.c b/Python/traceback.c index d289cd3ffaa6f4..16b6ee72a74f97 100644 --- a/Python/traceback.c +++ b/Python/traceback.c @@ -552,7 +552,6 @@ display_source_line_with_margin(PyObject *f, PyObject *filename, int lineno, int } assert(!err); - assert(lineobj); if (_Py_WriteIndentedMargin(margin_indent, margin, f) < 0) { goto error; From a05ac6321cc8aeb68c7451996fdf4f6a6ebe22fb Mon Sep 17 00:00:00 2001 From: Irit Katriel Date: Tue, 7 Dec 2021 11:33:44 +0000 Subject: [PATCH 6/6] Erlend's latest comments --- Python/traceback.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/Python/traceback.c b/Python/traceback.c index 16b6ee72a74f97..b0ff5e9a6b0757 100644 --- a/Python/traceback.c +++ b/Python/traceback.c @@ -406,16 +406,14 @@ int _Py_WriteIndentedMargin(int indent, const char *margin, PyObject *f) { if (_Py_WriteIndent(indent, f) < 0) { - goto error; + return -1; } if (margin) { if (PyFile_WriteString(margin, f) < 0) { - goto error; + return -1; } } return 0; -error: - return -1; } static int @@ -423,7 +421,6 @@ display_source_line_with_margin(PyObject *f, PyObject *filename, int lineno, int int margin_indent, const char *margin, int *truncation, PyObject **line) { - int err = 0; int fd; int i; char *found_encoding; @@ -506,7 +503,6 @@ display_source_line_with_margin(PyObject *f, PyObject *filename, int lineno, int lineobj = PyFile_GetLine(fob, -1); if (!lineobj) { PyErr_Clear(); - err = -1; break; } } @@ -520,7 +516,7 @@ display_source_line_with_margin(PyObject *f, PyObject *filename, int lineno, int Py_DECREF(fob); if (!lineobj || !PyUnicode_Check(lineobj)) { Py_XDECREF(lineobj); - return err; + return -1; } if (line) { @@ -551,8 +547,6 @@ display_source_line_with_margin(PyObject *f, PyObject *filename, int lineno, int *truncation = i - indent; } - assert(!err); - if (_Py_WriteIndentedMargin(margin_indent, margin, f) < 0) { goto error; }