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

(MODULES-1967) Parse escape sequences from iptables #513

Conversation

karmix
Copy link
Contributor

@karmix karmix commented Apr 23, 2015

Interpret escape sequences in all string arguments for iptables options.

@jonnytdevops
Copy link
Contributor

Thanks for your efforts on this.

It's seems very risky to run this new backslash handling code through the entire rule, rather than just the comment block.

Perhaps you could re-write your PR by limiting your code to the --comment field?

Thanks!

@karmix
Copy link
Contributor Author

karmix commented Apr 23, 2015

That was my original intent, however, I found that there was no way to accomplish this without significant changes to the parser. The current parser extracts the hash keys from the value string, tokenizes the remaining values, then decides which token to assign to each key. It is not possible to tokenize arguments differently for each option, if you don't know which option you are trying to parse. We are left with a choice, either change the scan RE and affect tokenization for all options (risky), or rewrite the parser and try to translate the existing logic (affects a lot more, but would also address other open bugs).

Without a rewrite, the token RE needs to change; it is not possible to restructure the values after they are already split and put into the hash because the scan loses white space. After the scan RE is changed, the parser will assign hash values such as 'one " two', where previously it would only assign 'one '. If an option does not support escape characters in its strings, the token has too many characters, regardless of whether we interpret the escape sequences. We can't fix this case without a rewrite. If the option does support escape characters in its strings, the token is wrong until we interpret the escape characters.

The proper solution is to redesign the parser. That should involve some coordination. This change for the existing parser simply moves the bug to what I believe to be a smaller (probably non-existent) set of edge cases.

@jonnytdevops
Copy link
Contributor

Hi Karmix,

Sorry about the long delay.

I think I'm prepared to merge your commit however it would be appreciated if you could update the code with a fairly detailed comment on what it's actually doing, as I'm finding it a bit tricky to follow (although I understand the purpose by reading the ticket).

Thanks

@karmix
Copy link
Contributor Author

karmix commented Jun 6, 2015

JT:

The past few days have been really busy for me. I just wanted to let you
know that I got your request, and I hope to look into it next week.

Thanks,
Doug
On Jun 3, 2015 5:32 AM, "JT (Jonny)" notifications@github.com wrote:

Hi Karmix,

Sorry about the long delay.

I think I'm prepared to merge your commit however it would be appreciated
if you could update the code with a fairly detailed comment on what it's
actually doing, as I'm finding it a bit tricky to follow (although I
understand the purpose by reading the ticket).

Thanks


Reply to this email directly or view it on GitHub
#513 (comment)
.

@karmix karmix force-pushed the tickets/master/MODULES-1967_parse-escape-sequences-from-iptables branch from 57ddba6 to 16cfbdc Compare June 10, 2015 19:47
@karmix
Copy link
Contributor Author

karmix commented Jun 10, 2015

JT:

I have updated the comment for the main hash generation (and squashed the commit). Is this what you wanted?

Thanks,
Doug

@jonnytdevops
Copy link
Contributor

Awesome, thanks!

jonnytdevops added a commit that referenced this pull request Jun 18, 2015
…-escape-sequences-from-iptables

(MODULES-1967) Parse escape sequences from iptables
@jonnytdevops jonnytdevops merged commit 1c85e10 into puppetlabs:master Jun 18, 2015
cegeka-jenkins pushed a commit to cegeka/puppet-firewall that referenced this pull request Oct 23, 2017
…1967_parse-escape-sequences-from-iptables

(MODULES-1967) Parse escape sequences from iptables
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants