Skip to content

Fix dohpath parsing/stringify#1366

Merged
miekg merged 3 commits intomiekg:masterfrom
shane-kerr:fix-dohpath-string
May 10, 2022
Merged

Fix dohpath parsing/stringify#1366
miekg merged 3 commits intomiekg:masterfrom
shane-kerr:fix-dohpath-string

Conversation

@shane-kerr
Copy link
Copy Markdown
Contributor

I noticed that the brand-new dohpath support has a similar problem to the one addressed in PR #1361 - non-printable characters are not handled properly.

Luckily the parameter is much simpler than the alpn parameter. I refactored the parse/stringify SVCBLocal out and use that for both types.

Comment thread svcb.go Outdated
s.Template = b
template, err := svcbParseParam(b)
if err != nil {
return fmt.Errorf("dns: svcblocal: svcb private/experimental key %w", err)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This message should probably be about svcbdohpath?

Copy link
Copy Markdown
Contributor Author

@shane-kerr shane-kerr Apr 14, 2022

Choose a reason for hiding this comment

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

Whoops! Cut & paste error. I've fix this, thanks for spotting!

@miekg
Copy link
Copy Markdown
Owner

miekg commented Apr 14, 2022

@shane-kerr probably makes sense to rework #1363 to make that more generic to handle all this stuff with 1 or 2 functions? Or does this obsolete that PR?

@shane-kerr
Copy link
Copy Markdown
Contributor Author

@shane-kerr probably makes sense to rework #1363 to make that more generic to handle all this stuff with 1 or 2 functions? Or does this obsolete that PR?

@miekg when I looked at #1363 I considered using a single function for parsing the alpn and the local stuff, but the encoded comma-separated list means that the rules are actually quite different. On my first attempt, I used the SVCBLocal version and then tried decoding and splitting the result, but it was both inefficient and messy. I could possibly try that again, but I'm pretty happy with the resulting code. Similarly the stringify needs to encode any , and \ that it encounters, so it's not really directly usable.

This PR is different, since the handling for parsing and stringify is actually identical for the dohpath and the local stuff. I'm pretty sure the two PR don't collide, so can both be merged usefully.

If we really want to refactor code to remove redundancy, we could look at pulling out the escaping functionality in sprintTxt() into a separate routine that operates on a single string, and doesn't add " around it. Then we could eliminate the svcbParamToStr() function I introduced completely. There may also be some clever way to get the parsing from existing code, although that's likely more work. Let me know what you think.

@miekg
Copy link
Copy Markdown
Owner

miekg commented May 10, 2022

I'll hold off merging, until #1363 is in, might result in conflicts otherwise?

@shane-kerr
Copy link
Copy Markdown
Contributor Author

Yes, makes sense. I don't expect conflicts, but we'll see!

@miekg miekg merged commit bfcbf0f into miekg:master May 10, 2022
aanm pushed a commit to cilium/dns that referenced this pull request Jul 29, 2022
* dohpath escaped in String(), and parsed such values

* Update the test for dohpath with escaping

* Fix cut & paste error with svcdohpath error
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

Successfully merging this pull request may close these issues.

4 participants