#38 graphite2 causing libreoffice segfault

[Anonymous]

Passing along a bug report that's come through the fedora bug reporting system. (Original bug report here: https://bugzilla.redhat.com/show_bug.cgi?id=752569 ). From that bug report, it is suggested that:

"I strongly suspect that it is a bug in impl. of graphite2::Vector. "designed to have a limited subset of the std::vector api". Seriously?! The mind boggles... Just from a peek into the impl., Vector::swap() has two serious bugs: it does not swap (because it takes its argument by value!) and causes double free."

[Martin Hosken]

Thank you for spotting the missing & in the Vector::swap signature (which accounts for the two bugs pointed out). It should be noted that we can find no evidence of this function ever getting called anywhere either in graphite or in libo graphite integration, and if it were, we would expect it to blow up in fuzz testing rather quickly. So I don't think that's the problem.

Looking at the stack trace in the referenced bug, there are things we don't understand. For example, why is graphite_serverfontlayout.cxx in vcl/source/glyphs when in my libo source it is in vcl/generic/glyphs? Where the fault occurs in assembler, implies that gr_feature_destroy has been called with a reference into bad memory. Even doubly freed memory is unlikely to segfault like this. This puts the bug up in libo land.

Can you provide a test document so that we can try to recreate the problem?

[David Tardon]

> It should be noted that we can find
> no evidence of this function ever getting called anywhere either in
> graphite or in libo graphite integration, and if it were, we would expect
> it to blow up in fuzz testing rather quickly. So I don't think that's the
> problem.

Neither do I, but I spent about 30 seconds looking at the code in List.h and found 2 bugs, so it just seemed reasonable to think there might be others.

> Thank you for spotting the missing & in the Vector::swap signature (which
> accounts for the two bugs pointed out).

This is actually only one bug and just adding the ampersand immediately introduces another one. Let me categorize it:

1. Vector::swap does not swap
after x.swap(y), x == y because of missing ampersand in the function's signature

2. vec is destroyed twice
after the last memcpy tmp has the same content as vec. tmp is destroyed when it goes out of scope, _destroying_ the data that vec points to.

3. self-swap is not handled correctly
x.swap(x) leads to call of memcpy on overlapping ranges (actualy equal ranges), which has undefined behaviour

I suggest that you use the usual idiom

void Vector<T>::swap(Vector<T>& vec)
{
using std::swap;
swap(m_first, vec.m_first);
swap(m_last, vec.m_last);
swap(m_end, vec.m_end);
}

which is both correct and more efficient than your implementation.

(Actually, I suggest that you just use std::vector...)