pyOpenSSL

Merge lp://staging/~zseil/pyopenssl/util-cleanup into lp://staging/~exarkun/pyopenssl/trunk

  1. util-cleanup
  2. Merge into trunk
Proposed by Ziga Seilnacht
Status: Needs review
Proposed branch: lp://staging/~zseil/pyopenssl/util-cleanup
Merge into: lp://staging/~exarkun/pyopenssl/trunk
Diff against target: 96 lines (+40/-19)
2 files modified
src/util.c (+40/-18)
src/util.h (+0/-1)
To merge this branch: bzr merge lp://staging/~zseil/pyopenssl/util-cleanup
Reviewer Review Type Date Requested Status
Jean-Paul Calderone Pending
Review via email: mp+16514@code.staging.launchpad.net
To post a comment you must log in.
Revision history for this message
Ziga Seilnacht (zseil) wrote :

Here is the branch that adds error checks to the functions in util.c. The important changes are in the first changeset, subsequent changesets fix minor nits and can be dropped.

Unfortunately, I didn't find any reasonable way to test these changes. The errors would only manifest in low memory conditions, and I don't know of any tools that would reliably cause a memory allocation failure at an exact time (especially when you take Python's obmalloc into account). Maybe we could submit pyOpenSSL to Coverity's Scan <http://scan.coverity.com/>, I think that their static analysis tool is able to detect missing NULL checks.

Reply

Unmerged revisions

130. By Ziga Seilnacht

Add documentation for exception_from_error_queue.

129. By Ziga Seilnacht

error_queue_to_list is not used outside of util.c, so it can be static.

128. By Ziga Seilnacht

Flush OpenSSL's error queue even when list allocation fails.

127. By Ziga Seilnacht

Avoid memory allocation to prevent possible loss of error details.

E.g. before this change code like this might overwrite the exception
from callback with uninformative MemoryError:

    pkey = PEM_read_bio_PrivateKey(bio, NULL, callback, cb_arg);
    if (pkey == NULL) {
        if (PyErr_Occured()) {
            /* Reraise the Python level error from callback */
            flush_error_queue();
            return NULL;
        }
    }

126. By Ziga Seilnacht

Add missing error checks.

I don't know of any reasonable way to test these changes,
but it is really unfriendly to segfault while trying to raise an exception.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/util.c'
2--- src/util.c 2009-09-01 14:35:50 +0000
3+++ src/util.c 2009-12-22 22:41:13 +0000
4@@ -19,29 +19,53 @@
5 * Arguments: None
6 * Returns: A list of errors (new reference)
7 */
8-PyObject *
9+static PyObject *
10 error_queue_to_list(void) {
11 PyObject *errlist, *tuple;
12+ int failed;
13 long err;
14
15 errlist = PyList_New(0);
16-
17+ if (errlist == NULL) {
18+ goto error;
19+ }
20 while ((err = ERR_get_error()) != 0) {
21- tuple = Py_BuildValue("(sss)", ERR_lib_error_string(err),
22- ERR_func_error_string(err),
23- ERR_reason_error_string(err));
24- PyList_Append(errlist, tuple);
25+ tuple = Py_BuildValue("(sss)", ERR_lib_error_string(err),
26+ ERR_func_error_string(err),
27+ ERR_reason_error_string(err));
28+ if (tuple == NULL) {
29+ Py_DECREF(errlist);
30+ goto error;
31+ }
32+ failed = PyList_Append(errlist, tuple);
33 Py_DECREF(tuple);
34+ if (failed) {
35+ Py_DECREF(errlist);
36+ goto error;
37+ }
38 }
39-
40 return errlist;
41+
42+error:
43+ flush_error_queue();
44+ return NULL;
45 }
46
47-void exception_from_error_queue(PyObject *the_Error) {
48+/*
49+ * Set a Python level exception from OpenSSL's current error queue.
50+ *
51+ * Arguments: The exception type to raise. It will be called with a list
52+ * of tuples, describing the OpenSSL exception.
53+ * Returns: None
54+ */
55+void exception_from_error_queue(PyObject *the_Error) {
56 PyObject *errlist = error_queue_to_list();
57- PyErr_SetObject(the_Error, errlist);
58- Py_DECREF(errlist);
59-}
60+
61+ if (errlist != NULL) {
62+ PyErr_SetObject(the_Error, errlist);
63+ Py_DECREF(errlist);
64+ }
65+}
66
67 /*
68 * Flush OpenSSL's error queue and ignore the result
69@@ -51,11 +75,9 @@
70 */
71 void
72 flush_error_queue(void) {
73- /*
74- * Make sure to save the errors to a local. Py_DECREF might expand such
75- * that it evaluates its argument more than once, which would lead to
76- * very nasty things if we just invoked it with error_queue_to_list().
77- */
78- PyObject *list = error_queue_to_list();
79- Py_DECREF(list);
80+ long err;
81+
82+ do {
83+ err = ERR_get_error();
84+ } while (err);
85 }
86
87=== modified file 'src/util.h'
88--- src/util.h 2009-08-31 23:32:29 +0000
89+++ src/util.h 2009-12-22 22:41:13 +0000
90@@ -22,7 +22,6 @@
91 */
92 #include "pymemcompat.h"
93
94-extern PyObject *error_queue_to_list(void);
95 extern void exception_from_error_queue(PyObject *the_Error);
96 extern void flush_error_queue(void);
97

Subscribers

People subscribed via source and target branches

to status/vote changes: