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

urlapi: return CURLUE_BAD_HOSTNAME if puny2idn encoding fails #11674

Closed
wants to merge 6 commits into from

Conversation

bagder
Copy link
Member

@bagder bagder commented Aug 15, 2023

And document it

@bagder bagder added the URL label Aug 15, 2023
@bagder bagder mentioned this pull request Aug 15, 2023
@bagder
Copy link
Member Author

bagder commented Aug 15, 2023

Alternatives:

  1. have the flag be a "best effort" and just return it as-is if it fails (but that then doesn't tell the user if it worked)
  2. introduce a new error code to make the IDN conversion failure more explicit
  3. keep this PR

@bagder
Copy link
Member Author

bagder commented Aug 15, 2023

@jacobmealey thoughts?

@jacobmealey
Copy link

jacobmealey commented Aug 15, 2023

it looks like idn2_to_unicode_8z8z(puny, &enc, 0) is never throwing an error when passed some malformed punycode. It should return IDN2_ENCODING_ERROR but its returning 0 (IDN2_OK) and an empty string. I've only done preliminary testing but that's what I've noticed.

note that I'm using libidn2.so.0.3.8
and I can reproduce the behavior with this example program: https://gitlab.com/libidn/libidn2/-/blob/master/examples/example-tounicode.c

@bagder
Copy link
Member Author

bagder commented Aug 15, 2023

I think maybe it is harder to make up a bad punycode than I considered. I now tested with xn---------- and it made idn2_to_unicode_8z8z() return -202 for me.

@bagder
Copy link
Member Author

bagder commented Aug 15, 2023

we could of course also remove the check for the xn-- prefix and then trow a string at it that does not start with xn--...

@jacobmealey
Copy link

Okay, I'm getting the same results with using xn----------, so thats good. it may be good to have curl_url_get return the same error for Curl_idn_decode, because right now if that fails it reports out of memory and not bad host name.

@bagder
Copy link
Member Author

bagder commented Aug 16, 2023

it may be good to have curl_url_get return the same error for Curl_idn_decode

Ack. I'll make sure that it returns CURLUE_BAD_HOSTNAME for bad host names and CURLUE_OUT_OF_MEMORY only when it actually runs out of memory.

lib/idn.c Outdated Show resolved Hide resolved
@bagder bagder closed this in a281057 Aug 17, 2023
@bagder bagder deleted the bagder/urlget-idn-encode branch August 17, 2023 06:22
ptitSeb pushed a commit to wasix-org/curl that referenced this pull request Sep 25, 2023
And document it. Only return out of memory when it actually is a memory
problem.

Pointed-out-by: Jacob Mealey
Closes curl#11674
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

2 participants