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

[pkg/ottl] Grammar cannot handle multiple string parameters where the first string ends with \\" #23238

Closed
TylerHelmuth opened this issue Jun 8, 2023 · 5 comments
Labels
bug Something isn't working never stale Issues marked with this label will be never staled and automatically removed pkg/ottl priority:p1 High

Comments

@TylerHelmuth
Copy link
Member

TylerHelmuth commented Jun 8, 2023

Component(s)

pkg/ottl

What happened?

Description

The exact statement, simplified, that does not work is a("\\", "b"). This should be interpreted as an Editor named a with two string parameters, "\\", and "v". But instead the parser fails complaining about backslashes.

This happens because the lexer token we use to identify strings is matching too much content from the incoming statement. The String token is

`"(\\"|[^"])*"`

Since we use backticks, this will match a double quote, followed by 0 or more exactly 2 backslashes followed by a double quote OR anything that isn't a double quote, followed by a double quote. You can see the regex in action with the troublesome text here: https://regex101.com/r/oqpxQ4/1.

What is happening is that the parser is treating \\" as literal backslash, backslash, double quote, and so it matches the first part of the token and since there is another double quote later on that is the greediest match possible. This was confirmed by debugging the lexer token selection.

I don't have a good solution to fix this yet. The \\" exists in the token to allow double quotes in the eventually strconv.Unquoted strings. Removing the \\" part of the regex does solve the problem, but doesn't let use to do statements like set(body, "the cat said \\"meow\\""), which seems limiting.

To be clear, this is only an issue if there is a string parameter following a string parameter that ends in \\". Statements like a("\\") work.

At the time of writing this Issue the following ottlfuncs functions are affected:

  • replace_match
  • replace_all_matches
  • replace_pattern
  • replace_all_patterns
  • IsMatch
  • Split

Workaround

For now, the workaround is to place any character after the \\. In this case if you're trying to match a single backslash your regex could be "\\{1}".

Steps to Reproduce

Try to run the transformprocessor with a("\\", "b") as a statement.

Collector version

v0.79.0

@TylerHelmuth TylerHelmuth added bug Something isn't working needs triage New item requiring triage priority:p1 High pkg/ottl and removed needs triage New item requiring triage labels Jun 8, 2023
@TylerHelmuth TylerHelmuth changed the title [pkg/ottl] Grammar cannot handle multiple string parameters where the first string ends with \\" [pkg/ottl] Grammar cannot handle multiple string parameters where the first string ends with \\" Jun 9, 2023
@TylerHelmuth
Copy link
Member Author

The root cause of the error is actually when the parser calles strconv.Unquote.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 9, 2023

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@github-actions github-actions bot added the Stale label Aug 9, 2023
@TylerHelmuth TylerHelmuth added never stale Issues marked with this label will be never staled and automatically removed and removed Stale labels Aug 9, 2023
@quentinmit
Copy link
Contributor

I think this regex would fix the problem:

"(\\.|[^\\"])*"

That is, match a backslash followed by any character, or any character that isn't a backslash or double-quote.

@TylerHelmuth
Copy link
Member Author

@quentinmit I did some real quick testing and I think you're right. Would you like to submit a PR to fix this issue?

@quentinmit
Copy link
Contributor

@TylerHelmuth Sure, I just sent #30839

quentinmit added a commit to quentinmit/opentelemetry-collector-contrib that referenced this issue Jan 31, 2024
TylerHelmuth pushed a commit that referenced this issue Jan 31, 2024
**Description:** <Describe what has changed.>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
Fix ottl parsing to properly handle escape sequences.
@TylerHelmuth

**Link to tracking Issue:**
#23238

**Testing:** <Describe what testing was performed and which tests were
added.>
Added a new unit test with the example from the linked issue

**Documentation:** <Describe the documentation added.>
None (pure bugfix)
cparkins pushed a commit to AmadeusITGroup/opentelemetry-collector-contrib that referenced this issue Feb 1, 2024
**Description:** <Describe what has changed.>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
Fix ottl parsing to properly handle escape sequences.
@TylerHelmuth

**Link to tracking Issue:**
open-telemetry#23238

**Testing:** <Describe what testing was performed and which tests were
added.>
Added a new unit test with the example from the linked issue

**Documentation:** <Describe the documentation added.>
None (pure bugfix)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working never stale Issues marked with this label will be never staled and automatically removed pkg/ottl priority:p1 High
Projects
None yet
Development

No branches or pull requests

2 participants