Skip to content

Make path extension a bit safer #30208

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Make path extension a bit safer #30208

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8736a98
Make path-to-string conversion a little safer
QuLogic Jun 24, 2025
e0dfd27
Make path clipping a bit safer
QuLogic Jun 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 30 additions & 35 deletions src/_path.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,12 @@
#ifndef MPL_PATH_H
#define MPL_PATH_H

#include <limits>
#include <math.h>
#include <vector>
#include <cmath>
#include <algorithm>
#include <array>
#include <cmath>
#include <limits>
#include <string>
#include <vector>

#include "agg_conv_contour.h"
#include "agg_conv_curve.h"
Expand Down Expand Up @@ -524,12 +524,11 @@ struct bisectx
{
}

inline void bisect(double sx, double sy, double px, double py, double *bx, double *by) const
inline auto bisect(double sx, double sy, double px, double py) const
{
*bx = m_x;
double dx = px - sx;
double dy = py - sy;
*by = sy + dy * ((m_x - sx) / dx);
return std::tuple(m_x, sy + dy * ((m_x - sx) / dx));
}
};

Expand Down Expand Up @@ -565,12 +564,11 @@ struct bisecty
{
}

inline void bisect(double sx, double sy, double px, double py, double *bx, double *by) const
inline auto bisect(double sx, double sy, double px, double py) const
{
*by = m_y;
double dx = px - sx;
double dy = py - sy;
*bx = sx + dx * ((m_y - sy) / dy);
return std::tuple(sx + dx * ((m_y - sy) / dy), m_y);
}
};

Expand Down Expand Up @@ -615,8 +613,7 @@ inline void clip_to_rect_one_step(const Polygon &polygon, Polygon &result, const
pinside = filter.is_inside(px, py);

if (sinside ^ pinside) {
double bx, by;
filter.bisect(sx, sy, px, py, &bx, &by);
auto [bx, by] = filter.bisect(sx, sy, px, py);
result.emplace_back(bx, by);
}

Expand Down Expand Up @@ -1051,15 +1048,14 @@ void cleanup_path(PathIterator &path,
void quad2cubic(double x0, double y0,
double x1, double y1,
double x2, double y2,
double *outx, double *outy)
std::array<double, 3> &outx, std::array<double, 3> &outy)
{

outx[0] = x0 + 2./3. * (x1 - x0);
outy[0] = y0 + 2./3. * (y1 - y0);
outx[1] = outx[0] + 1./3. * (x2 - x0);
outy[1] = outy[0] + 1./3. * (y2 - y0);
outx[2] = x2;
outy[2] = y2;
std::get<0>(outx) = x0 + 2./3. * (x1 - x0);
std::get<0>(outy) = y0 + 2./3. * (y1 - y0);
std::get<1>(outx) = std::get<0>(outx) + 1./3. * (x2 - x0);
std::get<1>(outy) = std::get<0>(outy) + 1./3. * (y2 - y0);
std::get<2>(outx) = x2;
std::get<2>(outy) = y2;
}


Expand Down Expand Up @@ -1104,27 +1100,27 @@ void __add_number(double val, char format_code, int precision,
template <class PathIterator>
bool __convert_to_string(PathIterator &path,
int precision,
char **codes,
const std::array<std::string, 5> &codes,
bool postfix,
std::string& buffer)
{
const char format_code = 'f';

double x[3];
double y[3];
std::array<double, 3> x;
std::array<double, 3> y;
double last_x = 0.0;
double last_y = 0.0;

unsigned code;

while ((code = path.vertex(&x[0], &y[0])) != agg::path_cmd_stop) {
while ((code = path.vertex(&std::get<0>(x), &std::get<0>(y))) != agg::path_cmd_stop) {
Copy link
Contributor

@anntzer anntzer Jun 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think you can still do &x[0] no? (or x.at(0) if you really want bounds checking here; this still reads better than std::get I'd say)

Copy link
Member Author

Choose a reason for hiding this comment

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

std::get is compile-time checked for constants; neither x[0] nor x.at(0) are unfortunately.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ugh, indeed, that's a bit annoying...

if (code == CLOSEPOLY) {
buffer += codes[4];
buffer += std::get<4>(codes);
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

} else if (code < 5) {
size_t size = NUM_VERTICES[code];

for (size_t i = 1; i < size; ++i) {
unsigned subcode = path.vertex(&x[i], &y[i]);
unsigned subcode = path.vertex(&x.at(i), &y.at(i));
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the compiler can safely elide the bounds check here, because it'll have trouble proving that size is small enough (I guess the "modern C++" way of ensuring that is to make NUM_VERTICES an int templated on code etc.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, x.at is the bounds-checked version, and x[i] isn't, but somehow the compiled code remains the same size either way. (Perhaps this is because the Fedora compiler has hardening enabled somewhere?)

if (subcode != code) {
return false;
}
Expand All @@ -1133,29 +1129,29 @@ bool __convert_to_string(PathIterator &path,
/* For formats that don't support quad curves, convert to
cubic curves */
if (code == CURVE3 && codes[code - 1][0] == '\0') {
quad2cubic(last_x, last_y, x[0], y[0], x[1], y[1], x, y);
quad2cubic(last_x, last_y, x.at(0), y.at(0), x.at(1), y.at(1), x, y);
code++;
size = 3;
}

if (!postfix) {
buffer += codes[code - 1];
buffer += codes.at(code - 1);
buffer += ' ';
}

for (size_t i = 0; i < size; ++i) {
__add_number(x[i], format_code, precision, buffer);
__add_number(x.at(i), format_code, precision, buffer);
buffer += ' ';
__add_number(y[i], format_code, precision, buffer);
__add_number(y.at(i), format_code, precision, buffer);
buffer += ' ';
}

if (postfix) {
buffer += codes[code - 1];
buffer += codes.at(code - 1);
}

last_x = x[size - 1];
last_y = y[size - 1];
last_x = x.at(size - 1);
last_y = y.at(size - 1);
} else {
// Unknown code value
return false;
Expand All @@ -1174,7 +1170,7 @@ bool convert_to_string(PathIterator &path,
bool simplify,
SketchParams sketch_params,
int precision,
char **codes,
const std::array<std::string, 5> &codes,
bool postfix,
std::string& buffer)
{
Expand Down Expand Up @@ -1211,7 +1207,6 @@ bool convert_to_string(PathIterator &path,
sketch_t sketch(curve, sketch_params.scale, sketch_params.length, sketch_params.randomness);
return __convert_to_string(sketch, precision, codes, postfix, buffer);
}

}

template<class T>
Expand Down
7 changes: 1 addition & 6 deletions src/_path_wrapper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,16 +252,11 @@ static py::object
Py_convert_to_string(mpl::PathIterator path, agg::trans_affine trans,
agg::rect_d cliprect, std::optional<bool> simplify,
SketchParams sketch, int precision,
std::array<std::string, 5> codes_obj, bool postfix)
const std::array<std::string, 5> &codes, bool postfix)
{
char *codes[5];
std::string buffer;
bool status;

for (auto i = 0; i < 5; ++i) {
codes[i] = const_cast<char *>(codes_obj[i].c_str());
}

if (!simplify.has_value()) {
simplify = path.should_simplify();
}
Expand Down
Loading