Skip to content

urlapi: return CURLUE_BAD_HOSTNAME if puny2idn encoding fails#11674

Closed
bagder wants to merge 6 commits intomasterfrom
bagder/urlget-idn-encode
Closed

urlapi: return CURLUE_BAD_HOSTNAME if puny2idn encoding fails#11674
bagder wants to merge 6 commits intomasterfrom
bagder/urlget-idn-encode

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
Contributor

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
Contributor

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.

@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.

2 participants