8000 BUG: non integer overlap might lead to corrupt memory access in as_strided [backport 1.4.x] by mbyt · Pull Request #3845 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

BUG: non integer overlap might lead to corrupt memory access in as_strided [backport 1.4.x] #3845

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 26, 2014
Merged

BUG: non integer overlap might lead to corrupt memory access in as_strided [backport 1.4.x] #3845

merged 1 commit into from
Nov 26, 2014

Conversation

mbyt
Copy link
Contributor
@mbyt mbyt commented Nov 25, 2014

Running mlab.psd with non integer noverlap might lead to corrupt memory access in the underlying np.lib.stride_tricks.as_strides. This is not obvious to debug, especially when noverlap is calculated programmatic.

Example:

from __future__ import division
import numpy as np
import matplotlib.pyplot as plt

N = 1000
n_sin_periods = 10

x = np.empty(3*N, dtype='f4')
x.fill(np.NaN)
y = x[N:2*N]
y[:] = np.sin(np.arange(N)/N * 2 * np.pi * n_sin_periods)

plt.psd(y, NFFT=330, noverlap=0.6)
plt.show()

This was not problematic with matplotlib 0.13, as the mlab.psd implementation was not using np.lib.stride_tricks.as_strides at that time.

tacaswell
tacaswell reviewed Nov 25, 2014
View reviewed changes
lib/matplotlib/mlab.py
@@ -590,6 +590,19 @@ def stride_windows(x, n, noverlap=None, axis=0):
if n > x.size:
raise ValueError('n cannot be greater than the length of x')

# as_strided can lead to memory out of bound access for non
# integer noverlap or n
if not isinstance(noverlap, (int, long)):
Copy link
Member

Choose a reason for hiding this comment

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

Does this catch all of possible numpy scalar types?

Copy link
Member

Choose a reason for hiding this comment

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

A non-integer overlap is really an error, and not a very likely one. I think a simpler solution might be better: no deprecation, just use noverlap = int(noverlap) to force it to be a python integer.

Copy link
Member

Choose a reason for hiding this comment

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

I was in the process of writing the same thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this catch all of possible numpy scalar types?

Good point, it probably does not catch all possible numpy scalar types.

and not a very likely one

Didn't think about that --- good point! I fully agree. I will remove the deprecation warning and directly cast to int.

@tacaswell tacaswell changed the title BUG: ensure integer shape, strides for as_strided BUG: ensure integer shape, strides for as_strided [backport 1.4.x] Nov 25, 2014
@tacaswell tacaswell added this to the v1.4.3 milestone Nov 25, 2014
@mbyt mbyt force-pushed the out_of_memory_access_as_strided branch from d34a3f0 to 43fd6be Compare November 26, 2014 19:21
@mbyt
Copy link
Contributor Author
mbyt commented Nov 26, 2014

Alright, I updated as mentioned followed by a git rebase and push --force to remove the clutter.

I also improved and simplified the test case.

@mbyt mbyt force-pushed the out_of_memory_access_as_strided branch from 43fd6be to 81492ad Compare November 26, 2014 19:32
@mbyt
Copy link
Contributor Author
mbyt commented Nov 26, 2014

Sorry and yet another test improvement lead to the above rebase 81492ad.

@tacaswell
Copy link
Member

No worries. Pending travis passing this looks good to me.

tacaswell added a commit that referenced this pull request Nov 26, 2014
@tacaswell
Merge pull request #3845 from mbyt/out_of_memory_access_as_strided
df3530d 
BUG: ensure integer shape, strides for as_strided
@tacaswell tacaswell merged commit df3530d into matplotlib:master Nov 26, 2014
tacaswell added a commit that referenced this pull request Nov 26, 2014
@tacaswell
Merge pull request #3845 from mbyt/out_of_memory_access_as_strided
edd2a87 
BUG: ensure integer shape, strides for as_strided
@tacaswell
Copy link
Member

back ported as edd2a87

@mbyt
Copy link
Contributor Author
mbyt commented Nov 26, 2014

That was fast! Thanks and keep up the good work!

@mbyt mbyt deleted the out_of_memory_access_as_strided branch November 26, 2014 21:06
@mbyt
Copy link
Contributor Author
mbyt commented Dec 29, 2014

Just for documentation, the problem were non integer overlaps, which lead to strides, whose size was smaller than the actual data type (i 7FF4 n the example below, 1byte stride, for a 4byte float).

In [23]: [(n, hex(np.array([n], dtype='>f4').view('>u4'))) for n in np.lib.stride_tricks.as_strided(np.array([1.27953441e-28]*2, dtype='>f4'), shape=(4,), strides=(1,))]
Out[23]: 
[(1.2795344e-28, '0x11223344L'),
 (2.4295058e-18, '0x22334411L'),
 (4.5650388e-08, '0x33441122L'),
 (580.53436, '0x44112233L')]

I clarified the title correspondingly.

@mbyt mbyt changed the title BUG: ensure integer shape, strides for as_strided [backport 1.4.x] BUG: non integer overlap might lead to corrupt memory access in as_strided [backport 1.4.x] Dec 29, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.4.3
Development

Successfully merging this pull request may close these issues.
3 participants
@mbyt @tacaswell @efiring
0