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

Fix segfault in ft2font #5084

Merged
merged 2 commits into from Sep 18, 2015
Merged

Fix segfault in ft2font #5084

merged 2 commits into from Sep 18, 2015

Conversation

cgohlke
Copy link
Contributor

@cgohlke cgohlke commented Sep 15, 2015

On Windows, Python 2.7, 32 and 64 bit, the sankey_demo_rankine.py example segfaults in the ft2font.pyd extension at https://github.com/matplotlib/matplotlib/blob/master/src/ft2font_wrapper.cpp#L667. Apparently on that platform std::vector of size 0 doesn't like to be indexed.
Please review: I did not check if not calling CALL_CPP("set_text", self->x->set_text(size, &codepoints[0], angle, flags, xys)) for size 0 has any side effects.

@cgohlke cgohlke closed this Sep 15, 2015
@cgohlke cgohlke reopened this Sep 15, 2015
@tacaswell tacaswell added this to the next point release (1.5.0) milestone Sep 16, 2015
@tacaswell
Copy link
Member

Another reason to release more often is we will get @cgohlke to look at our code more often....

@tacaswell
Copy link
Member

and we really need to get appveyor setup.

@jkseppan
Copy link
Member

Calling set_text with an empty text has the side effect of setting the bounding box to empty. One way font objects are used seems to be that you call font.set_text(...) and then font.get_width_height(), and if the objects get used many times, I guess the object could remember the bounding box of the previous call and give a nonempty size for an empty string.

@@ -664,7 +668,9 @@ static PyObject *PyFT2Font_set_text(PyFT2Font *self, PyObject *args, PyObject *k
return NULL;
}

CALL_CPP("set_text", self->x->set_text(size, &codepoints[0], angle, flags, xys));
if (size > 0) {
CALL_CPP("set_text", self->x->set_text(size, &codepoints[0], angle, flags, xys));
Copy link
Member

Choose a reason for hiding this comment

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

So the problem is that codepoints[0] is illegal for an empty vector? If we can guarantee a C++11 compiler (probably not?) you could use codepoints.data(). Otherwise, I guess something like this would work:

uint32_t* codepoints_array = NULL;
if (size > 0) {
    codepoints_array = &codepoints[0];
}
CALL_CPP(... codepoints_array, ...));

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 know how much we can rely on C++11 -- we'd have to test it. I think gcc is fine, but clang and MSVC may be playing catch up still (not sure).

Anyway, your workaround seems like a good suggestion. I suspect what's happening here is that no buffer is allocated at all in the zero-length case, so &codepoints[0] is garbage.

@jkseppan
Copy link
Member

Looks good to me, and passes tests. Did you check that this still fixes the segfault on Windows?

@cgohlke
Copy link
Contributor Author

cgohlke commented Sep 18, 2015

Yes, I tested this on Windows.

jkseppan added a commit that referenced this pull request Sep 18, 2015
@jkseppan jkseppan merged commit 8f53402 into matplotlib:master Sep 18, 2015
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

4 participants