FIX: segfault on truncated png #9257
Conversation
- be more paranoid in _read_png_data about failed reads and check for exceptions after both calls to it - in read_png_data kick the png error handling - only override the exception in the body of `setjmp` handler if there is not already one set closes matplotlib#9256
src/_png.cpp
Outdated
| PyErr_SetString(PyExc_IOError, "read past end of file"); | ||
| } | ||
| } | ||
| if (result) { |
There was a problem hiding this comment.
All the code uses some mixed tab/space indent. It should be 4-space indent even in the C code, I believe.
src/_png.cpp
Outdated
| } else { | ||
| PyErr_SetString(PyExc_IOError, "Failed to copy buffer"); | ||
| } | ||
| } else { |
src/_png.cpp
Outdated
| @@ -424,6 +432,11 @@ static void read_png_data(png_structp png_ptr, png_bytep data, png_size_t length | |||
| { | |||
| PyObject *py_file_obj = (PyObject *)png_get_io_ptr(png_ptr); | |||
| _read_png_data(py_file_obj, data, length); | |||
| if (PyErr_Occurred()) | |||
| { | |||
src/_png.cpp
Outdated
| @@ -481,6 +494,9 @@ static PyObject *_read_png(PyObject *filein, bool float_result) | |||
| } | |||
| Py_XDECREF(read_method); | |||
| _read_png_data(py_file, header, 8); | |||
| if (PyErr_Occurred()){ | |||
src/_png.cpp
Outdated
| @@ -503,7 +519,10 @@ static PyObject *_read_png(PyObject *filein, bool float_result) | |||
| } | |||
|
|
|||
| if (setjmp(png_jmpbuf(png_ptr))) { | |||
| PyErr_SetString(PyExc_RuntimeError, "Error setting jump"); | |||
| if (!PyErr_Occurred()) | |||
| { | |||
src/_png.cpp
Outdated
| @@ -424,6 +432,11 @@ static void read_png_data(png_structp png_ptr, png_bytep data, png_size_t length | |||
| { | |||
| PyObject *py_file_obj = (PyObject *)png_get_io_ptr(png_ptr); | |||
| _read_png_data(py_file_obj, data, length); | |||
| if (PyErr_Occurred()) | |||
There was a problem hiding this comment.
-Wmisleading-indentation :-) (here and below)
|
I do not do a lot of c-code and the auto-formatter in emacs was fighting back.... |
lib/matplotlib/tests/test_png.py
Outdated
| @@ -46,3 +47,30 @@ def test_imread_png_uint16(): | |||
|
|
|||
| assert (img.dtype == np.uint16) | |||
| assert np.sum(img.flatten()) == 134184960 | |||
|
|
|||
|
|
|||
| def test_truncated_file(): | |||
There was a problem hiding this comment.
Do we really need to test both filenames and streams? (or rather, there doesn't seem to be a test of successfully reading from a stream (as opposed to a filename), which seems something more valuable to check...)
There was a problem hiding this comment.
Yes, because they go through different code paths.
There was a problem hiding this comment.
but not in the erroring part right? (at that point every possible input has been converted to an object with a read method)
There was a problem hiding this comment.
🐑 Yup, I was confused be the _read_png_data in the first if / else block and thought that was reading the whole file, but is just pulling off the first 8 bytes.
lib/matplotlib/tests/test_png.py
Outdated
| with pytest.raises(Exception): | ||
| plt.imread(fname_t) | ||
|
|
||
| shutil.rmtree(d) |
There was a problem hiding this comment.
This should be in finally, or maybe use the tempdir pytest fixture.
|
@antongulikov why? |
|
If anything, error messages should start with a lower case letter and not have final dots for consistency with CPython itself IMO... (e.g., |
|
@tacaswell I wanted to say that the style for error messages should be the same for different exceptions. |
d9b604c to
6ebcb11
Compare
|
@antongulikov That makes sense, I think we have made the all consistent now. |
PR Summary
exceptions after both calls to it
setjmphandler ifthere is not already one set
closes #9256
PR Checklist
OP has a test case, just need to add it.