From 742499a20b99bc1d6d103c7a16376c4a2ef053a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabian=20Ke=C3=9Fler?= Date: Fri, 11 Oct 2019 09:41:38 +0200 Subject: [PATCH] Fixed undefined behaviour by replacing dynamic allocated and resized c-array with a std::vector --- src/model/Point.h | 2 + src/model/Stroke.cpp | 178 +++++++++++++++---------------------------- src/model/Stroke.h | 14 ++-- 3 files changed, 73 insertions(+), 121 deletions(-) diff --git a/src/model/Point.h b/src/model/Point.h index 7e48cb79..7d4333ba 100644 --- a/src/model/Point.h +++ b/src/model/Point.h @@ -32,6 +32,8 @@ public: */ Point(const Point& p); + Point& operator=(Point const&) = default; + /** * @brief Point from two values. * @param x X value of the point. diff --git a/src/model/Stroke.cpp b/src/model/Stroke.cpp index a544c6aa..c79b5521 100644 --- a/src/model/Stroke.cpp +++ b/src/model/Stroke.cpp @@ -6,6 +6,7 @@ #include #include +#include Stroke::Stroke() : AudioElement(ELEMENT_STROKE) @@ -17,11 +18,6 @@ Stroke::~Stroke() { XOJ_CHECK_TYPE(Stroke); - g_free(this->points); - this->points = NULL; - this->pointCount = 0; - this->pointAllocCount = 0; - XOJ_RELEASE_TYPE(Stroke); } @@ -45,11 +41,7 @@ Stroke* Stroke::cloneStroke() const Stroke* s = new Stroke(); s->applyStyleFrom(this); - - s->allocPointSize(this->pointCount); - memcpy(s->points, this->points, this->pointCount * sizeof(Point)); - s->pointCount = this->pointCount; - + s->points = this->points; return s; } @@ -74,7 +66,7 @@ void Stroke::serialize(ObjectOutputStream& out) out.writeInt(fill); - out.writeData(this->points, this->pointCount, sizeof(Point)); + out.writeData(this->points.data(), this->points.size(), sizeof(Point)); this->lineStyle.serialize(out); @@ -95,11 +87,11 @@ void Stroke::readSerialized(ObjectInputStream& in) this->fill = in.readInt(); - g_free(this->points); - this->points = NULL; - this->pointCount = 0; - in.readData((void**) &this->points, &this->pointCount); - + Point* p; + int count{}; + in.readData((void**) &p, &count); + this->points = std::vector{p, p + count}; + delete[] p; this->lineStyle.readSerialized(in); in.endObject(); @@ -151,10 +143,10 @@ bool Stroke::isInSelection(ShapeContainer* container) { XOJ_CHECK_TYPE(Stroke); - for (int i = 0; i < this->pointCount; i++) + for (auto&& p: this->points) { - double px = this->points[i].x; - double py = this->points[i].y; + double px = p.x; + double py = p.y; if (!container->contains(px, py)) { @@ -169,9 +161,9 @@ void Stroke::setFirstPoint(double x, double y) { XOJ_CHECK_TYPE(Stroke); - if (this->pointCount > 0) + if (!this->points.empty()) { - Point& p = this->points[0]; + Point& p = this->points.front(); p.x = x; p.y = y; this->sizeCalculated = false; @@ -181,114 +173,77 @@ void Stroke::setFirstPoint(double x, double y) void Stroke::setLastPoint(double x, double y) { XOJ_CHECK_TYPE(Stroke); - - if (this->pointCount > 0) - { - Point& p = this->points[this->pointCount - 1]; - p.x = x; - p.y = y; - this->sizeCalculated = false; - } + setLastPoint({x, y}); } -void Stroke::setLastPoint(Point p) +void Stroke::setLastPoint(const Point& p) { - setLastPoint(p.x, p.y); -} - -void Stroke::addPoint(Point p) -{ - XOJ_CHECK_TYPE(Stroke); - - if (this->pointCount >= this->pointAllocCount - 1) + if (!this->points.empty()) { - this->allocPointSize(this->pointAllocCount + 100); + this->points.back() = p; } - this->points[this->pointCount++] = p; - this->sizeCalculated = false; } -void Stroke::allocPointSize(int size) +void Stroke::addPoint(const Point& p) { XOJ_CHECK_TYPE(Stroke); - this->pointAllocCount = size; - this->points = (Point*) g_realloc(this->points, this->pointAllocCount * sizeof(Point)); + this->points.emplace_back(p); + this->sizeCalculated = false; } int Stroke::getPointCount() const { XOJ_CHECK_TYPE(Stroke); - return this->pointCount; + return this->points.size(); } ArrayIterator Stroke::pointIterator() const { XOJ_CHECK_TYPE(Stroke); - return ArrayIterator (points, pointCount); + return ArrayIterator(points.data(), points.size()); } void Stroke::deletePointsFrom(int index) { XOJ_CHECK_TYPE(Stroke); - if (this->pointCount <= index) - { - return; - } - this->pointCount = index; + points.resize(std::min(size_t(index), points.size())); } void Stroke::deletePoint(int index) { XOJ_CHECK_TYPE(Stroke); - if (this->pointCount <= index) - { - return; - } - - for (int i = 0; i < this->pointCount; i++) - { - if (i >= index) - { - this->points[i] = this->points[i + 1]; - } - } - this->pointCount--; + this->points.erase(std::next(begin(this->points), index)); } Point Stroke::getPoint(int index) const { XOJ_CHECK_TYPE(Stroke); - if (index < 0 || index >= pointCount) + if (index < 0 || index >= this->points.size()) { g_warning("Stroke::getPoint(%i) out of bounds!", index); return Point(0, 0, Point::NO_PRESSURE); } - return points[index]; + return points.at(index); } const Point* Stroke::getPoints() const { XOJ_CHECK_TYPE(Stroke); - return this->points; + return this->points.data(); } void Stroke::freeUnusedPointItems() { XOJ_CHECK_TYPE(Stroke); - if (this->pointAllocCount == this->pointCount) - { - return; - } - this->pointAllocCount = this->pointCount + 1; - this->points = (Point*) g_realloc(this->points, this->pointAllocCount * sizeof(Point)); + this->points = {begin(this->points), end(this->points)}; } void Stroke::setToolType(StrokeTool type) @@ -323,10 +278,10 @@ void Stroke::move(double dx, double dy) { XOJ_CHECK_TYPE(Stroke); - for (int i = 0; i < pointCount; i++) + for (auto&& point: points) { - points[i].x += dx; - points[i].y += dy; + point.x += dx; + point.y += dy; } this->sizeCalculated = false; @@ -335,11 +290,9 @@ void Stroke::move(double dx, double dy) void Stroke::rotate(double x0, double y0, double xo, double yo, double th) { XOJ_CHECK_TYPE(Stroke); - - for (int i = 0; i < this->pointCount; i++) - { - Point& p = this->points[i]; + for (auto&& p: points) + { p.x -= x0; //move to origin p.y -= y0; double offset = 0.7; // __DBL_EPSILON__; @@ -354,8 +307,8 @@ void Stroke::rotate(double x0, double y0, double xo, double yo, double th) p.x += x0; //restore the position p.y += y0; - p.x += xo-offset; //center it - p.y += yo-offset; + p.x += xo - offset; //center it + p.y += yo - offset; } //Width and Height will likely be changed after this operation calcSize(); @@ -367,10 +320,8 @@ void Stroke::scale(double x0, double y0, double fx, double fy) double fz = sqrt(fx * fy); - for (int i = 0; i < this->pointCount; i++) + for (auto&& p: points) { - Point& p = this->points[i]; - p.x -= x0; p.x *= fx; p.x += x0; @@ -393,7 +344,7 @@ bool Stroke::hasPressure() const { XOJ_CHECK_TYPE(Stroke); - if (this->pointCount > 0) + if (!this->points.empty()) { return this->points[0].z != Point::NO_PRESSURE; } @@ -403,12 +354,9 @@ bool Stroke::hasPressure() const double Stroke::getAvgPressure() const { XOJ_CHECK_TYPE(Stroke); - double summatory = 0; - for (int i = 0; i < this->pointCount; i++) - { - summatory += this->points[i].z; - } - return summatory / this->pointCount; + return std::accumulate(begin(this->points), end(this->points), 0.0, + [](double l, Point const& p) { return l + p.z; }) / + this->points.size(); } void Stroke::scalePressure(double factor) @@ -419,9 +367,9 @@ void Stroke::scalePressure(double factor) { return; } - for (int i = 0; i < this->pointCount; i++) + for (auto&& p: this->points) { - this->points[i].z *= factor; + p.z *= factor; } } @@ -429,9 +377,9 @@ void Stroke::clearPressure() { XOJ_CHECK_TYPE(Stroke); - for (int i = 0; i < this->pointCount; i++) + for (auto&& p: points) { - this->points[i].z = Point::NO_PRESSURE; + p.z = Point::NO_PRESSURE; } } @@ -439,9 +387,9 @@ void Stroke::setLastPressure(double pressure) { XOJ_CHECK_TYPE(Stroke); - if (this->pointCount > 0) + if (!this->points.empty()) { - this->points[this->pointCount - 1].z = pressure; + this->points.back().z = pressure; } } @@ -450,13 +398,14 @@ void Stroke::setPressure(const vector& pressure) XOJ_CHECK_TYPE(Stroke); // The last pressure is not used - as there is no line drawn from this point - if (this->pointCount - 1 > (int)pressure.size()) + if (this->points.size() - 1 != pressure.size()) { - g_warning("invalid pressure point count: %i, expected %i", (int)pressure.size(), (int)this->pointCount - 1); - return; + g_warning("invalid pressure point count: %s, expected %s", std::to_string(pressure.size()).data(), + std::to_string(this->points.size() - 1).data()); } - for (int i = 0; i < this->pointCount && i < (int)pressure.size(); i++) + auto max_size = std::min(pressure.size(), this->points.size() - 1); + for (size_t i = 0U; i != max_size; ++i) { this->points[i].z = pressure[i]; } @@ -479,7 +428,7 @@ bool Stroke::intersects(double x, double y, double halfEraserSize, double* gap) { XOJ_CHECK_TYPE(Stroke); - if (this->pointCount < 1) + if (this->points.empty()) { return false; } @@ -491,10 +440,10 @@ bool Stroke::intersects(double x, double y, double halfEraserSize, double* gap) double lastX = points[0].x; double lastY = points[0].y; - for (int i = 1; i < pointCount; i++) + for (auto&& point: points) { - double px = points[i].x; - double py = points[i].y; + double px = point.x; + double py = point.y; if (px >= x1 && py >= y1 && px <= x2 && py <= y2) { @@ -556,7 +505,7 @@ void Stroke::calcSize() { XOJ_CHECK_TYPE(Stroke); - if (this->pointCount == 0) + if (this->points.empty()) { Element::x = 0; Element::y = 0; @@ -574,15 +523,15 @@ void Stroke::calcSize() bool hasPressure = points[0].z != Point::NO_PRESSURE; double halfThick = this->width / 2.0; // accommodate for pen width - for (int i = 0; i < this->pointCount; i++) + for (auto&& p: points) { - if (hasPressure) halfThick = points[i].z / 2.0; + if (hasPressure) halfThick = p.z / 2.0; - minX = std::min(minX, points[i].x - halfThick); - minY = std::min(minY, points[i].y - halfThick); + minX = std::min(minX, p.x - halfThick); + minY = std::min(minY, p.y - halfThick); - maxX = std::max(maxX, points[i].x + halfThick); - maxY = std::max(maxY, points[i].y + halfThick); + maxX = std::max(maxX, p.x + halfThick); + maxY = std::max(maxY, p.y + halfThick); } Element::x = minX - 2; @@ -611,9 +560,8 @@ void Stroke::debugPrint() g_message("%s", FC(FORMAT_STR("Stroke {1} / hasPressure() = {2}") % (uint64_t) this % this->hasPressure())); - for (int i = 0; i < this->pointCount; i++) + for (auto&& p: points) { - Point p = this->points[i]; g_message("%lf / %lf", p.x, p.y); } diff --git a/src/model/Stroke.h b/src/model/Stroke.h index d0769b76..3d8be58c 100644 --- a/src/model/Stroke.h +++ b/src/model/Stroke.h @@ -29,6 +29,11 @@ class Stroke : public AudioElement { public: Stroke(); + Stroke(Stroke const&) = default; + Stroke(Stroke&&) = default; + + Stroke& operator=(Stroke const&) = default; + Stroke& operator=(Stroke&&) = default; virtual ~Stroke(); public: @@ -61,10 +66,10 @@ public: */ void setFill(int fill); - void addPoint(Point p); + void addPoint(const Point& p); void setLastPoint(double x, double y); void setFirstPoint(double x, double y); - void setLastPoint(Point p); + void setLastPoint(const Point& p); int getPointCount() const; void freeUnusedPointItems(); ArrayIterator pointIterator() const; @@ -109,7 +114,6 @@ public: protected: virtual void calcSize(); - void allocPointSize(int size); private: XOJ_TYPE_ATTRIB; @@ -120,9 +124,7 @@ private: StrokeTool toolType = STROKE_TOOL_PEN; // The array with the points - Point* points = NULL; - int pointCount = 0; - int pointAllocCount = 0; + std::vector points{}; /** * Dashed line