On 7/7/2015 6:33 PM, Daniel Hardman wrote:
> 1. The pull request is from my private fork to Ray's
> check_utf8_before_libidn branch; it doesn't yet target master. This is
> because I'd like at least Ray and Daniel's feedback, first.
>
> 2. I'm a bit unhappy about the lack of error propagation. Right now,
> if we detect invalid utf8 in a hostname, we record the error, but the
> only downstream consequence is that we leave non-ascii in ->host, and
> then allow DNS to fail. As a result, invalid utf8 manifests with
> CURLE_COULDNT_RESOLVE_HOST. Should we return something more specific?
>
> 3. Related to Q2: is the unit test adequate? Right now it proves we
> react to invalid utf8 in hostname by failing to connect--but that
> error condition is so generic that it could be caused by almost
> anything. Is there a better way?
>
> 4. Please also note my odd tweak to handle the (sometimes) static
> utf8len() function. Is this kosher?

I've been busy all week and I'd hoped to have some time to review and
test your patch [1] before answering these questions but I haven't. I
did do a cursory review and added the patch as a draft to the branch
earlier this week. I messed up tests/unit/Makefile.inc though when I
edited the patch, I see now. Oops. It's ok though these are just drafts
so with regard to 1 it wouldn't go in master anyway right now. And
recent developments this week mean it may not go in at all, which I'll
get to in a moment, but first to answer your other questions:

wrt 2: Resolving a host that could not be converted to utf-8 and then to
idn it was clearly designed that way. You are making an assumption DNS
will fail. I assume it was done that way so that the resolver can have a
last chance crack at it (ie some resolvers have their own translation
facility?). The error message I don't have a problem with, but I don't
know where everyone stands on that. Judging by the lack of response
though I'd say it's fine.

wrt 3: The unit test of the utf8 check function has one problem I can
see which is you created the easy handle twice and that should show up
as a memory leak so that would need to be fixed. Also, actually testing
to make sure we catch a hostname that can't be converted to utf-8 would
be better probably if it was broken out into a separate test. In other
words one unit test to test the function and another unit test to make
sure the code in url works as expected and catches invalid utf-8. To
make sure the function is working we'd probably need a test that
monitors the output for '* Hostname contains invalid UTF-8 sequence'
instead of something broad like ret != 0. I think the <verify> xml in
test data may be able to be used for that, I'm not sure.

wrt 4: I moved utf8len to non-ascii.c and renamed the function to
utf8_strict_codepoint_count in other drafts so you don't have to worry
about that.

On to recent developments:

On 7/10/2015 6:06 AM, Daniel Stenberg wrote:
> Since libidn now shipped a fixed version, do we still need this check
> or any portion of it?

libidn which I didn't get the impression would address this issue has
since addressed it [2] using the u8-check module from gnulib. Note the
module allows for utf-8 that is not well-formed, also known as modified
utf-8 or other. That should be sufficient, though the check I wrote is
much more strict in that it only allows well-formed utf-8 to pass.

So it looks like the branch [3] may be moot, unless someone there are
some strong opinions (anyone?) on why we need stricter checking than
libidn. I'm willing to part with the branch because I'm loathe to add
code unnecessarily and cause bloat.

Thanks again for your help Daniel Hardman.

[1]: https://github.com/bagder/curl/pull/340
[2]:
http://git.savannah.gnu.org/cgit/libidn.git/commit/?id=2e97c2796581c27213962c77f5a8571a598f9a2e
[3]:
https://github.com/bagder/curl/compare/check_utf8_before_libidn?expand=1

-------------------------------------------------------------------
List admin: http://cool.haxx.se/list/listinfo/curl-library
Etiquette: http://curl.haxx.se/mail/etiquette.html
Received on 2015-07-10