-
Notifications
You must be signed in to change notification settings - Fork 226
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
Heap buffer overflow in Polynomial: #156
Comments
I propose we put the |
template <class U>
polynomial(const U* data, unsigned order)
: m_data(data, data + order + 1)
{
normalize();
} The target constructor does not seem so intuitive. The interface only makes sense when I open the source and see that order of the polynomial is requested here. If I somehow knew that the second parameter is order, then using polynomial<double> p(coeffs.data(), coeffs.size() - 1) is quite natural for the consumer/caller of this constructor IMHO. What I would consider idiomatic(for better of a better wording, read "programmers are used to it") is to pass a raw pointer and a corresponding length. In that case, template <class U>
polynomial(const U* data, unsigned offset)
: m_data(data, data + offset)
{
normalize();
} seem to be making more sense for the general public but nonetheless as error-prone as the other approach. |
Agreed; I think it's not idiomatic. Adding the deprecation would move this to C++-14 compatibility only though; I don't think it's possible to change the current behavior. @jzmaddock : Would a C++-11 break to recommend the move constructor be ok for this one? |
Well... that parameter is clearly labelled as "order" in the docs, though I confess the class is otherwise rather under-documented :(
That function has always been that way, so I kind of hesitate to deprecate it. However, adding a deprecate attribute when in C++14 would do not harm, even if it was just to emphasise the point that the second parameter is polynomial order not number of terms. |
I agree with @NAThompson that it's not an intuitive interface, since a constructor for a container-esque class with that signature is a pretty widely established convention to mean (first, size). |
Code to reproduce:
Compile with AddressSanitizer:
Then:
The constructor call should be
polynomial<double> p(coeffs.data(), coeffs.size() -1)
, but this doesn't seem idiomatic to me. Is there any way we can assert on this?The text was updated successfully, but these errors were encountered: