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

BUG: Showing a BboxImage can cause a segmentation fault #3009

Merged
merged 2 commits into from May 16, 2014

Conversation

solvents
Copy link
Contributor

This is in response to issue #2963. It turns out that BboxImage's make_image method could potentially calculate a negative width or height if it was constructed from an "inverted" Bbox, which is then passed to Image::resize.

From there, the width and height are interpreted as unsigned by agg, which leads to a segfault.
Small example:

from matplotlib.image import BboxImage, imread
from matplotlib.transforms import Bbox
import matplotlib.pyplot as plt

ax = plt.axes()
bbox_image = BboxImage(Bbox([[100, 100], [0, 0]]))
path = '/usr/share/pixmaps/firefox.png'
image = imread(path)
bbox_image.set_data(image)
ax.add_artist(bbox_image)
plt.show()

This revealed another problem, in BboxImage.show, since (once fixed), inverting the order of the two corners passed into the Bbox constructor changes where the image is rendered.

Image::resize will now raise a runtime error if the user passes in an invalid width or height.
BboxImage.make_image will now ensure that the width and height it passes to Image::resize are positive or zero.
BboxImage.draw now calculates the bottom left corner of its Bbox, rather than using the "first" corner of its Bbox.
@tacaswell
Copy link
Member

This looks good to me. Could you add a test for this? It does not need to check the result image, just make sure it runs. Pasting that example code in to the test suite should be good enough.

@cleanup
def test_bbox_invert():
     blah

@mdboom Thoughts? I am a bit worried that there seems to be a int -> unit cast someplace which we are not handling properly. The sanity checks are good, but should it be dealt with else where?

@solvents
Copy link
Contributor Author

I ended up using an image comparison since I wasn't sure how to call show from a cleanup test (also, I don't think BboxImage had any tests).

Not sure if this will be helpful but the info I came across while debugging was:
The actual call to agg with the implicit cast was rbufOut->attach(bufferOut, numcols, numrows, numcols * BPP) (_image.cpp:392, attach defined in agg_rendering_buffer.h)
Backtrace (from the segfault):

#0  copy_hline (c=..., len=4294875116, y=0, x=0, this=0x7fffffffaa90)
    at extern/agg24/include/agg_pixfmt_rgba.h:1874
#1  clear (c=..., this=0x7fffffffaac0)
    at extern/agg24/include/agg_renderer_base.h:131
#2  Image::resize (this=0x1a4fc30, args=..., kwargs=...) at src/_image.cpp:397
#3  0x00007ffff236c145 in Py::PythonExtension<Image>::method_keyword_call_handler (_self_and_name_tuple=<optimized out>, _args=<optimized out>, 
    _keywords=<optimized out>) at extern/CXX/Python2/ExtensionOldType.hxx:321
...
(gdb) l
1869                ((value_type*)&v)[order_type::G] = c.g;
1870                ((value_type*)&v)[order_type::B] = c.b;
1871                ((value_type*)&v)[order_type::A] = c.a;
1872                do
1873                {
1874                    *(pixel_type*)p = v;
1875                    p += 4;
1876                }
1877                while(--len);
1878            }

agg aside, I think the problems begin at size_t NUMBYTES(numrows * numcols * BPP); (_image.cpp:381) since size_t is unsigned.

tacaswell added a commit that referenced this pull request May 16, 2014
BUG: Showing a BboxImage can cause a segmentation fault
@tacaswell tacaswell merged commit 9564d80 into matplotlib:master May 16, 2014
@tacaswell
Copy link
Member

@solvents Thanks for fixing this. I went ahead and merged it as-is as this patch fixes a bug we had and doesn't make the underlying issues of dangerous casts any worse (and we have probably had them for a long time).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants