Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Guard against memory unmapping errors #5775

Merged
merged 5 commits into from Feb 8, 2017

Conversation

mhvk
Copy link
Contributor

@mhvk mhvk commented Feb 7, 2017

This is an update of #4585, which ensures we don't get stack traces from badly corrupted compressed data hdus. It makes an additional change so that the code actually works as advertised (initializing *buf=NULL), simplifies it a bit, adds an extra error message, and adds tests.

fixes #3118

@mhvk
Copy link
Contributor Author

mhvk commented Feb 8, 2017

@MSeifert04, @pllim - I think this is OK -- at least it provable avoids getting a complete breakdown, stacktrace included -- but a closer look is most welcome...

@embray - if you have time, could you check that this indeed fixes #3118?

Copy link
Contributor

@MSeifert04 MSeifert04 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a two comments and a question but overall this looks good.

@@ -764,6 +768,13 @@ void init_output_buffer(PyObject* hdu, void** buf, size_t* bufsize) {
}

*buf = calloc(*bufsize, sizeof(char));
if (*buf == NULL) {
// Checking if calloc failed.
PyErr_SetString(PyExc_TypeError,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a PyExc_MemoryError and the next line has inconsistent indentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, makes sense, done.

@@ -888,7 +899,7 @@ PyObject* compression_compress_hdu(PyObject* self, PyObject* args)
PyObject* retval = NULL;
tcolumn* columns = NULL;

void* outbuf;
void* outbuf = NULL;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed? I'm only asking out of curiosity.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it turned out that was in fact the most important change I still had to make -- the problem is that init_output_buffer does not touch outbuf at all when an error occurs and then the comparison with NULL fails, leading to a later trial to unmap buf.

(I guess an alternative would be to check for PyErr_Occurred or to let init_output_buffer return or set a status flag.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see anything wrong just setting it to NULL anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed; overall, it seemed best to just keep any individual piece of code as little reliant as possible on the rest being correct...

CHANGES.rst Outdated
@@ -204,6 +204,9 @@ Bug Fixes

- Fix out-of-order TUNITn cards when writing tables to FITS. [#5720]

- Guard against extremely unlikely problems in compressed images, which
could lead to memory unmapping errors.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR number is missing. :)

@mhvk
Copy link
Contributor Author

mhvk commented Feb 8, 2017

OK, I fixed the indentation and added the PR number to the changelog. (I used [skip ci] since this should not change anything, but checked it locally just to be sure).

Ready to go in?

@pllim
Copy link
Member

pllim commented Feb 8, 2017

LGTM 👍 Thanks!

@mhvk mhvk merged commit 9c78d7f into astropy:master Feb 8, 2017
@mhvk mhvk deleted the io-fits-compressed-errors branch February 8, 2017 17:41
@MSeifert04
Copy link
Contributor

👍 Thanks

bsipocz pushed a commit that referenced this pull request Feb 14, 2017
Guard against memory unmapping errors
@bsipocz bsipocz modified the milestones: v1.0.12, v1.3.1 Feb 28, 2017
bsipocz pushed a commit that referenced this pull request Feb 28, 2017
Guard against memory unmapping errors
bsipocz pushed a commit to bsipocz/astropy that referenced this pull request Mar 5, 2017
bsipocz pushed a commit to bsipocz/astropy that referenced this pull request Mar 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A few unhandled errors in io.fits.compression module
5 participants