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.

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

Fix crash with simple string literal if-elif-else template #268

Open
tjsmith-meta opened this issue Sep 30, 2024 · 0 comments
Open

Fix crash with simple string literal if-elif-else template #268

tjsmith-meta opened this issue Sep 30, 2024 · 0 comments

Comments

@tjsmith-meta
Copy link

tjsmith-meta commented Sep 30, 2024

The following template loads and renders fine with Python jinja2, but crashes with jinja2cpp.

{% if 'foo' == 'bar'%}
1
{% elif 'foo' == 'foo' %}
2
{% else %}
3
{% endif %}

Note that removing elif, else will instead result in a load error (not a crash).

LOAD ERROR: noname.j2tpl:2:8: error: Expected expression, got: 'foo'
{% if 'foo' == 'bar'%}

Adding a space before '%}' on the first line will make the template actually function correctly.

Ideally, jinja2cpp would be able to handle this template just fine, like Python jinja2.

This looks like it fixes the crash and this example behaves correctly, although it's unclear to me whether this change might introduce other bugs. I noticed there was a similar check during end of expression for "'" (single quote) as well, so I remove that one too.

diff --git a/jinja2cpp/src/template_parser.h b/jinja2cpp/src/template_parser.h
--- a/jinja2cpp/src/template_parser.h
+++ b/jinja2cpp/src/template_parser.h
@@ -466,7 +466,7 @@
                     FinishCurrentLine(match.position() + 2);
                     return MakeParseError(ErrorCode::UnexpectedExprEnd, MakeToken(Token::ExprEnd, { matchStart, matchStart + 2 }));
                 }
-                else if (m_currentBlockInfo.type != TextBlockType::Expression || (*m_template)[match.position() - 1] == '\'')
+                else if (m_currentBlockInfo.type != TextBlockType::Expression)
                     break;

                 m_currentBlockInfo.range.startOffset = FinishCurrentBlock(matchStart, TextBlockType::RawText);
@@ -480,7 +480,7 @@
                     FinishCurrentLine(match.position() + 2);
                     return MakeParseError(ErrorCode::UnexpectedStmtEnd, MakeToken(Token::StmtEnd, { matchStart, matchStart + 2 }));
                 }
-                else if (m_currentBlockInfo.type != TextBlockType::Statement || (*m_template)[match.position() - 1] == '\'')
+                else if (m_currentBlockInfo.type != TextBlockType::Statement)
                     break;

                 m_currentBlockInfo.range.startOffset = FinishCurrentBlock(matchStart, TextBlockType::RawText);

Yeah, looks like string parsing is not handled fully correctly. Things like this appear to work fine in Python jinja2, but don't work in jinja2cpp (regardless of the code change above).

works in jinja2cpp without code change above but fails without
{% if '%}' == '%}' %}
1
{% endif %}

fails in jinja2cpp with or without code change above
{% if 'foo%}' == 'foo%}' %}
2
{% endif %}

It seems that the "check previous char" logic does not fully handle statement closing handling correctly, so removing that logic feels like the right thing to do.

Seems like we need to somehow detect that we are "inside a string literal" and ignore closing statement in such cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant