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 exception when calling resolveTxt with invalid unicode #199

Merged
merged 2 commits into from
Jul 19, 2024

Conversation

michalz-zoom
Copy link
Member

Sometimes resolveTxt throws when called with invalid unicode.

I couldn't repro on my system, neither with nodejs dns package, nor with native-dns that's used by Keybase server.

Some hints that other people have also been hitting this in the past: nodejs/node#39139

The solution is to wrap resolveTxt call in try/catch, and report like any normal error.

# the server)
dnslib = @libs.dns or dns
try
dnslib.resolveTxt domain, (err, records) ->
Copy link
Contributor

Choose a reason for hiding this comment

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

i didn't realize you could try/catch callback functions like this, cool!

Copy link
Member Author

Choose a reason for hiding this comment

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

you should also be able to do something like

try
   await foo defer err, res
catch err
cb err, res

but is silently broken in iced2 😢 IIRC the code after catch is not emitted at all for some reason

Copy link
Member Author

Choose a reason for hiding this comment

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

This only catches exceptions thrown in the sync part of resolveTxt though, if theres some exception in some callback afterwards, or even in the callback we supply here, it's not going to be caught by the try block.

@michalz-zoom michalz-zoom merged commit d026fa4 into master Jul 19, 2024
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.

2 participants