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.

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

muparser: avoid uncaught exception on invalid variable name #11793

Merged
merged 1 commit into from
Feb 5, 2025
Merged

muparser: avoid uncaught exception on invalid variable name #11793

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 7 additions & 0 deletions autotest/gdrivers/vrtderived.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1273,6 +1273,13 @@ def test_vrt_pixelfn_expression(
"exceeds maximum of 100000 set by GDAL_EXPRTK_MAX_EXPRESSION_LENGTH",
id="expression is too long",
),
pytest.param(
"B[1]",
[("B[1]", 3)],
"muparser",
"Invalid variable name",
id="invalid variable name",
),
],
)
def test_vrt_pixelfn_expression_invalid(
Expand Down
19 changes: 17 additions & 2 deletions frmts/vrt/vrtexpression_muparser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,17 +25,31 @@ class MuParserExpression::Impl
public:
explicit Impl(std::string_view osExpression)
: m_osExpression(std::string(osExpression)), m_oVectors{}, m_oParser{},
m_adfResults{1}, m_bIsCompiled{false}
m_adfResults{1}, m_bIsCompiled{false}, m_bCompileFailed{false}
{
}

void Register(std::string_view osVariable, double *pdfValue)
{
m_oParser.DefineVar(std::string(osVariable), pdfValue);
try
{
m_oParser.DefineVar(std::string(osVariable), pdfValue);
}
catch (const mu::Parser::exception_type &)
{
CPLError(CE_Failure, CPLE_AppDefined, "Invalid variable name: %s",
std::string(osVariable).c_str());
m_bCompileFailed = true;
Copy link
Member

Choose a reason for hiding this comment

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

no interesting error message coming from mu::Parser we could CPLError() ?

Copy link
Member Author

Choose a reason for hiding this comment

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

The message in the exception says that the variable name is invalid but doesn't say what the name was. Still, I've moved the site of the CPLError call to print an improved message.

Copy link
Member

Choose a reason for hiding this comment

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

Not to be addressed by this PR necessarily, but I'm wondering about the actual benefits of passing std::string_view as argument if we end up creating a std::string. This is slightly more verbose in term of source code, an probably also a bit in term of binary size ?

Copy link
Member Author

@dbaston dbaston Feb 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Actual benefit of std::string_view is negative, as you suggest. I think the longer-term improvement would be for CPLDebug and friends to accept a std::string_view argument instead of requiring everything be a C string. I haven't looked into it at all though, maybe there's some reason that won't work. Or maybe we should hold out for a couple of years and switch to std::format.

}
}

CPLErr Compile()
{
if (m_bCompileFailed)
{
return CE_Failure;
}

try
{
CPLString tmpExpression(m_osExpression);
Expand Down Expand Up @@ -99,6 +113,7 @@ class MuParserExpression::Impl
mu::Parser m_oParser;
std::vector<double> m_adfResults;
bool m_bIsCompiled;
bool m_bCompileFailed;
};

MuParserExpression::MuParserExpression(std::string_view osExpression)
Expand Down
Loading