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
=== modified file 'src/util.c'
--- src/util.c 2009-09-01 14:35:50 +0000
+++ src/util.c 2009-12-22 22:41:13 +0000
@@ -19,29 +19,53 @@
19 * Arguments: None19 * Arguments: None
20 * Returns: A list of errors (new reference)20 * Returns: A list of errors (new reference)
21 */21 */
22PyObject *22static PyObject *
23error_queue_to_list(void) {23error_queue_to_list(void) {
24 PyObject *errlist, *tuple;24 PyObject *errlist, *tuple;
25 int failed;
25 long err;26 long err;
2627
27 errlist = PyList_New(0);28 errlist = PyList_New(0);
2829 if (errlist == NULL) {
30 goto error;
31 }
29 while ((err = ERR_get_error()) != 0) {32 while ((err = ERR_get_error()) != 0) {
30 tuple = Py_BuildValue("(sss)", ERR_lib_error_string(err),33 tuple = Py_BuildValue("(sss)", ERR_lib_error_string(err),
31 ERR_func_error_string(err),34 ERR_func_error_string(err),
32 ERR_reason_error_string(err));35 ERR_reason_error_string(err));
33 PyList_Append(errlist, tuple);36 if (tuple == NULL) {
37 Py_DECREF(errlist);
38 goto error;
39 }
40 failed = PyList_Append(errlist, tuple);
34 Py_DECREF(tuple);41 Py_DECREF(tuple);
42 if (failed) {
43 Py_DECREF(errlist);
44 goto error;
45 }
35 }46 }
36
37 return errlist;47 return errlist;
48
49error:
50 flush_error_queue();
51 return NULL;
38}52}
3953
40void exception_from_error_queue(PyObject *the_Error) { 54/*
55 * Set a Python level exception from OpenSSL's current error queue.
56 *
57 * Arguments: The exception type to raise. It will be called with a list
58 * of tuples, describing the OpenSSL exception.
59 * Returns: None
60 */
61void exception_from_error_queue(PyObject *the_Error) {
41 PyObject *errlist = error_queue_to_list();62 PyObject *errlist = error_queue_to_list();
42 PyErr_SetObject(the_Error, errlist);63
43 Py_DECREF(errlist);64 if (errlist != NULL) {
44} 65 PyErr_SetObject(the_Error, errlist);
66 Py_DECREF(errlist);
67 }
68}
4569
46/*70/*
47 * Flush OpenSSL's error queue and ignore the result71 * Flush OpenSSL's error queue and ignore the result
@@ -51,11 +75,9 @@
51 */75 */
52void76void
53flush_error_queue(void) {77flush_error_queue(void) {
54 /*78 long err;
55 * Make sure to save the errors to a local. Py_DECREF might expand such79
56 * that it evaluates its argument more than once, which would lead to80 do {
57 * very nasty things if we just invoked it with error_queue_to_list().81 err = ERR_get_error();
58 */82 } while (err);
59 PyObject *list = error_queue_to_list();
60 Py_DECREF(list);
61}83}
6284
=== modified file 'src/util.h'
--- src/util.h 2009-08-31 23:32:29 +0000
+++ src/util.h 2009-12-22 22:41:13 +0000
@@ -22,7 +22,6 @@
22 */22 */
23#include "pymemcompat.h"23#include "pymemcompat.h"
2424
25extern PyObject *error_queue_to_list(void);
26extern void exception_from_error_queue(PyObject *the_Error);25extern void exception_from_error_queue(PyObject *the_Error);
27extern void flush_error_queue(void);26extern void flush_error_queue(void);
2827

Subscribers

People subscribed via source and target branches

to status/vote changes: