Update url_canon_icu prevent crash with GoogleUrl.

Update url_canon_icu prevent crash with GoogleUrl.

if GoogleUrl is constructed with a url like http://127.0.0.\230:8080/
a null-dereference can happen, prevent this from happening in a
non-debug build.

BUG=453508
https://code.google.com/p/chromium/issues/detail?id=453508

[jcrowell, 2015-02-03 20:39:21]

The CQ bit was checked by jcrowell@google.com

[jcrowell, 2015-02-03 20:39:22]

The CQ bit was unchecked by jcrowell@google.com

[jcrowell, 2015-03-27 16:37:48]

jcrowell@google.com changed reviewers:
+ jshin@chromium.org

[jcrowell, 2015-03-27 16:38:08]

On 2015/03/27 16:37:48, jcrowell wrote:
> mailto:jcrowell@google.com changed reviewers:
> + mailto:jshin@chromium.org

jshin@ ptal, thanks!

[jungshik at Google, 2015-03-27 21:59:16]

https://codereview.chromium.org/881073002/diff/20001/url/url_canon_icu.cc
File url/url_canon_icu.cc (right):

https://codereview.chromium.org/881073002/diff/20001/url/url_canon_icu.cc#new...
url/url_canon_icu.cc:168: }
This should not happen. If it happens, we have to track down the root cause. 
g_uidna is obtained here and I wonder why uidna_openUTS46 is failing. Is it
failing in Chrome? Moreover, it cannot depend on the input url.  

struct UIDNAWrapper {
  UIDNAWrapper() {
    UErrorCode err = U_ZERO_ERROR;
    // TODO(jungshik): Change options as different parties (browsers,
    // registrars, search engines) converge toward a consensus.
    value = uidna_openUTS46(UIDNA_CHECK_BIDI, &err);
    if (U_FAILURE(err))
      value = NULL;
  }

  UIDNA* value;
};

[jcrowell, 2015-03-30 14:17:03]

On 2015/03/27 21:59:16, Jungshik at google wrote:
> https://codereview.chromium.org/881073002/diff/20001/url/url_canon_icu.cc
> File url/url_canon_icu.cc (right):
> 
>
https://codereview.chromium.org/881073002/diff/20001/url/url_canon_icu.cc#new...
> url/url_canon_icu.cc:168: }
> This should not happen. If it happens, we have to track down the root cause. 
> g_uidna is obtained here and I wonder why uidna_openUTS46 is failing. Is it
> failing in Chrome? Moreover, it cannot depend on the input url.  
> 
> struct UIDNAWrapper {
>   UIDNAWrapper() {
>     UErrorCode err = U_ZERO_ERROR;
>     // TODO(jungshik): Change options as different parties (browsers,
>     // registrars, search engines) converge toward a consensus.
>     value = uidna_openUTS46(UIDNA_CHECK_BIDI, &err);
>     if (U_FAILURE(err))
>       value = NULL;
>   }
> 
>   UIDNA* value;
> };

I didn't observe this behavior in Chromium, but in modpagespeed. We construct a
GoogleUrl with a user-provided string which causes a crash.

[jcrowell, 2015-03-31 15:28:45]

On 2015/03/30 14:17:03, jcrowell wrote:
> On 2015/03/27 21:59:16, Jungshik at google wrote:
> > https://codereview.chromium.org/881073002/diff/20001/url/url_canon_icu.cc
> > File url/url_canon_icu.cc (right):
> > 
> >
>
https://codereview.chromium.org/881073002/diff/20001/url/url_canon_icu.cc#new...
> > url/url_canon_icu.cc:168: }
> > This should not happen. If it happens, we have to track down the root cause.

> > g_uidna is obtained here and I wonder why uidna_openUTS46 is failing. Is it
> > failing in Chrome? Moreover, it cannot depend on the input url.  
> > 
> > struct UIDNAWrapper {
> >   UIDNAWrapper() {
> >     UErrorCode err = U_ZERO_ERROR;
> >     // TODO(jungshik): Change options as different parties (browsers,
> >     // registrars, search engines) converge toward a consensus.
> >     value = uidna_openUTS46(UIDNA_CHECK_BIDI, &err);
> >     if (U_FAILURE(err))
> >       value = NULL;
> >   }
> > 
> >   UIDNA* value;
> > };
> 
> I didn't observe this behavior in Chromium, but in modpagespeed. We construct
a
> GoogleUrl with a user-provided string which causes a crash.

This is not just an issue with modpagespeed, but in any place that GoogleUrl is
constructed from user-supplied input, I just haven't tried to identify it in any
other places yet.

[jungshik at Google, 2015-04-01 08:00:09]

On 2015/03/31 15:28:45, jcrowell wrote:
> On 2015/03/30 14:17:03, jcrowell wrote:
> > On 2015/03/27 21:59:16, Jungshik at google wrote:
> > > https://codereview.chromium.org/881073002/diff/20001/url/url_canon_icu.cc
> > > File url/url_canon_icu.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/881073002/diff/20001/url/url_canon_icu.cc#new...
> > > url/url_canon_icu.cc:168: }
> > > This should not happen. If it happens, we have to track down the root
cause.
> 
> > > g_uidna is obtained here and I wonder why uidna_openUTS46 is failing. Is
it
> > > failing in Chrome? Moreover, it cannot depend on the input url.  
> > > 
> > > struct UIDNAWrapper {
> > >   UIDNAWrapper() {
> > >     UErrorCode err = U_ZERO_ERROR;
> > >     // TODO(jungshik): Change options as different parties (browsers,
> > >     // registrars, search engines) converge toward a consensus.
> > >     value = uidna_openUTS46(UIDNA_CHECK_BIDI, &err);
> > >     if (U_FAILURE(err))
> > >       value = NULL;
> > >   }
> > > 
> > >   UIDNA* value;
> > > };
> > 
> > I didn't observe this behavior in Chromium, but in modpagespeed. We
construct
> a
> > GoogleUrl with a user-provided string which causes a crash.
> 
> This is not just an issue with modpagespeed, but in any place that GoogleUrl
is
> constructed from user-supplied input, I just haven't tried to identify it in
any
> other places yet.

My question is how |uidna == null| can be input-dependent. (i.e how the failure
of |uidna_openUTS46| can depend on a user-input when it does not take any user
input).

[jcrowell, 2015-04-01 14:20:22]

On 2015/04/01 08:00:09, Jungshik at google wrote:
> On 2015/03/31 15:28:45, jcrowell wrote:
> > On 2015/03/30 14:17:03, jcrowell wrote:
> > > On 2015/03/27 21:59:16, Jungshik at google wrote:
> > > >
https://codereview.chromium.org/881073002/diff/20001/url/url_canon_icu.cc
> > > > File url/url_canon_icu.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/881073002/diff/20001/url/url_canon_icu.cc#new...
> > > > url/url_canon_icu.cc:168: }
> > > > This should not happen. If it happens, we have to track down the root
> cause.
> > 
> > > > g_uidna is obtained here and I wonder why uidna_openUTS46 is failing. Is
> it
> > > > failing in Chrome? Moreover, it cannot depend on the input url.  
> > > > 
> > > > struct UIDNAWrapper {
> > > >   UIDNAWrapper() {
> > > >     UErrorCode err = U_ZERO_ERROR;
> > > >     // TODO(jungshik): Change options as different parties (browsers,
> > > >     // registrars, search engines) converge toward a consensus.
> > > >     value = uidna_openUTS46(UIDNA_CHECK_BIDI, &err);
> > > >     if (U_FAILURE(err))
> > > >       value = NULL;
> > > >   }
> > > > 
> > > >   UIDNA* value;
> > > > };
> > > 
> > > I didn't observe this behavior in Chromium, but in modpagespeed. We
> construct
> > a
> > > GoogleUrl with a user-provided string which causes a crash.
> > 
> > This is not just an issue with modpagespeed, but in any place that GoogleUrl
> is
> > constructed from user-supplied input, I just haven't tried to identify it in
> any
> > other places yet.
> 
> My question is how |uidna == null| can be input-dependent. (i.e how the
failure
> of |uidna_openUTS46| can depend on a user-input when it does not take any user
> input).

I'm not really sure, spotted this bug a few months ago and made this patch to
our local copy without doing /too/ much investigation.
You can repro it by constructing a GURL with "http://127.0.0.\230:8080/" as the
argument.

If you'd like I can do some more digging, but it is definitely reproducible, and
any consumers of this who construct GURL(untrusted_user_input) will probably
experience the same crash.

[jungshik at Google, 2015-04-01 19:17:36]

On 2015/04/01 14:20:22, jcrowell wrote:
> On 2015/04/01 08:00:09, Jungshik at google wrote:
> > On 2015/03/31 15:28:45, jcrowell wrote:
> > > On 2015/03/30 14:17:03, jcrowell wrote:
> > > > On 2015/03/27 21:59:16, Jungshik at google wrote:
> > > > >
> https://codereview.chromium.org/881073002/diff/20001/url/url_canon_icu.cc
> > > > > File url/url_canon_icu.cc (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/881073002/diff/20001/url/url_canon_icu.cc#new...
> > > > > url/url_canon_icu.cc:168: }
> > > > > This should not happen. If it happens, we have to track down the root
> > cause.
> > > 
> > > > > g_uidna is obtained here and I wonder why uidna_openUTS46 is failing.
Is
> > it
> > > > > failing in Chrome? Moreover, it cannot depend on the input url.  
> > > > > 
> > > > > struct UIDNAWrapper {
> > > > >   UIDNAWrapper() {
> > > > >     UErrorCode err = U_ZERO_ERROR;
> > > > >     // TODO(jungshik): Change options as different parties (browsers,
> > > > >     // registrars, search engines) converge toward a consensus.
> > > > >     value = uidna_openUTS46(UIDNA_CHECK_BIDI, &err);
> > > > >     if (U_FAILURE(err))
> > > > >       value = NULL;
> > > > >   }
> > > > > 
> > > > >   UIDNA* value;
> > > > > };
> > > > 
> > > > I didn't observe this behavior in Chromium, but in modpagespeed. We
> > construct
> > > a
> > > > GoogleUrl with a user-provided string which causes a crash.
> > > 
> > > This is not just an issue with modpagespeed, but in any place that
GoogleUrl
> > is
> > > constructed from user-supplied input, I just haven't tried to identify it
in
> > any
> > > other places yet.
> > 
> > My question is how |uidna == null| can be input-dependent. (i.e how the
> failure
> > of |uidna_openUTS46| can depend on a user-input when it does not take any
user
> > input).
> 
> I'm not really sure, spotted this bug a few months ago and made this patch to
> our local copy without doing /too/ much investigation.
> You can repro it by constructing a GURL with "http://127.0.0.\230:8080/" as
the
> argument.
> 
> If you'd like I can do some more digging, but it is definitely reproducible,
and
> any consumers of this who construct GURL(untrusted_user_input) will probably
> experience the same crash.

Do you have a crash when you pass a perfectly normal url like
'http://www.google.com'? I suspect you do.

[jcrowell, 2015-04-01 19:23:52]

On 2015/04/01 19:17:36, Jungshik at google wrote:
> On 2015/04/01 14:20:22, jcrowell wrote:
> > On 2015/04/01 08:00:09, Jungshik at google wrote:
> > > On 2015/03/31 15:28:45, jcrowell wrote:
> > > > On 2015/03/30 14:17:03, jcrowell wrote:
> > > > > On 2015/03/27 21:59:16, Jungshik at google wrote:
> > > > > >
> > https://codereview.chromium.org/881073002/diff/20001/url/url_canon_icu.cc
> > > > > > File url/url_canon_icu.cc (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/881073002/diff/20001/url/url_canon_icu.cc#new...
> > > > > > url/url_canon_icu.cc:168: }
> > > > > > This should not happen. If it happens, we have to track down the
root
> > > cause.
> > > > 
> > > > > > g_uidna is obtained here and I wonder why uidna_openUTS46 is
failing.
> Is
> > > it
> > > > > > failing in Chrome? Moreover, it cannot depend on the input url.  
> > > > > > 
> > > > > > struct UIDNAWrapper {
> > > > > >   UIDNAWrapper() {
> > > > > >     UErrorCode err = U_ZERO_ERROR;
> > > > > >     // TODO(jungshik): Change options as different parties
(browsers,
> > > > > >     // registrars, search engines) converge toward a consensus.
> > > > > >     value = uidna_openUTS46(UIDNA_CHECK_BIDI, &err);
> > > > > >     if (U_FAILURE(err))
> > > > > >       value = NULL;
> > > > > >   }
> > > > > > 
> > > > > >   UIDNA* value;
> > > > > > };
> > > > > 
> > > > > I didn't observe this behavior in Chromium, but in modpagespeed. We
> > > construct
> > > > a
> > > > > GoogleUrl with a user-provided string which causes a crash.
> > > > 
> > > > This is not just an issue with modpagespeed, but in any place that
> GoogleUrl
> > > is
> > > > constructed from user-supplied input, I just haven't tried to identify
it
> in
> > > any
> > > > other places yet.
> > > 
> > > My question is how |uidna == null| can be input-dependent. (i.e how the
> > failure
> > > of |uidna_openUTS46| can depend on a user-input when it does not take any
> user
> > > input).
> > 
> > I'm not really sure, spotted this bug a few months ago and made this patch
to
> > our local copy without doing /too/ much investigation.
> > You can repro it by constructing a GURL with "http://127.0.0.\230:8080/" as
> the
> > argument.
> > 
> > If you'd like I can do some more digging, but it is definitely reproducible,
> and
> > any consumers of this who construct GURL(untrusted_user_input) will probably
> > experience the same crash.
> 
> Do you have a crash when you pass a perfectly normal url like
> 'http://www.google.com'? I suspect you do.

no, "normal" urls do not crash.

[jungshik at Google, 2015-04-01 20:09:40]

On 2015/04/01 19:17:36, Jungshik at google wrote:
> On 2015/04/01 14:20:22, jcrowell wrote:
> > On 2015/04/01 08:00:09, Jungshik at google wrote:
> > > On 2015/03/31 15:28:45, jcrowell wrote:
> > > > On 2015/03/30 14:17:03, jcrowell wrote:
> > > > > On 2015/03/27 21:59:16, Jungshik at google wrote:
> > > > > >
> > https://codereview.chromium.org/881073002/diff/20001/url/url_canon_icu.cc
> > > > > > File url/url_canon_icu.cc (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/881073002/diff/20001/url/url_canon_icu.cc#new...
> > > > > > url/url_canon_icu.cc:168: }
> > > > > > This should not happen. If it happens, we have to track down the
root
> > > cause.
> > > > 
> > > > > > g_uidna is obtained here and I wonder why uidna_openUTS46 is
failing.
> Is
> > > it
> > > > > > failing in Chrome? Moreover, it cannot depend on the input url.  
> > > > > > 
> > > > > > struct UIDNAWrapper {
> > > > > >   UIDNAWrapper() {
> > > > > >     UErrorCode err = U_ZERO_ERROR;
> > > > > >     // TODO(jungshik): Change options as different parties
(browsers,
> > > > > >     // registrars, search engines) converge toward a consensus.
> > > > > >     value = uidna_openUTS46(UIDNA_CHECK_BIDI, &err);
> > > > > >     if (U_FAILURE(err))
> > > > > >       value = NULL;
> > > > > >   }
> > > > > > 
> > > > > >   UIDNA* value;
> > > > > > };
> > > > > 
> > > > > I didn't observe this behavior in Chromium, but in modpagespeed. We
> > > construct
> > > > a
> > > > > GoogleUrl with a user-provided string which causes a crash.
> > > > 
> > > > This is not just an issue with modpagespeed, but in any place that
> GoogleUrl
> > > is
> > > > constructed from user-supplied input, I just haven't tried to identify
it
> in
> > > any
> > > > other places yet.
> > > 
> > > My question is how |uidna == null| can be input-dependent. (i.e how the
> > failure
> > > of |uidna_openUTS46| can depend on a user-input when it does not take any
> user
> > > input).
> > 
> > I'm not really sure, spotted this bug a few months ago and made this patch
to
> > our local copy without doing /too/ much investigation.
> > You can repro it by constructing a GURL with "http://127.0.0.\230:8080/" as
> the
> > argument.
> > 
> > If you'd like I can do some more digging, but it is definitely reproducible,
> and
> > any consumers of this who construct GURL(untrusted_user_input) will probably
> > experience the same crash.
> 
> Do you have a crash when you pass a perfectly normal url like
> 'http://www.google.com'? I suspect you do.

Maybe not that one because it's all ASCII. How about 'http://청와대.kr' ?

[jungshik at Google, 2015-04-09 18:26:24]

On 2015/04/01 20:09:40, Jungshik at google wrote:
> On 2015/04/01 19:17:36, Jungshik at google wrote:
> > On 2015/04/01 14:20:22, jcrowell wrote:
> > > On 2015/04/01 08:00:09, Jungshik at google wrote:
> > > > On 2015/03/31 15:28:45, jcrowell wrote:
> > > > > On 2015/03/30 14:17:03, jcrowell wrote:
> > > > > > On 2015/03/27 21:59:16, Jungshik at google wrote:
> > > > > > >
> > > https://codereview.chromium.org/881073002/diff/20001/url/url_canon_icu.cc
> > > > > > > File url/url_canon_icu.cc (right):
> > > > > > > 
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/881073002/diff/20001/url/url_canon_icu.cc#new...
> > > > > > > url/url_canon_icu.cc:168: }
> > > > > > > This should not happen. If it happens, we have to track down the
> root
> > > > cause.
> > > > > 
> > > > > > > g_uidna is obtained here and I wonder why uidna_openUTS46 is
> failing.
> > Is
> > > > it
> > > > > > > failing in Chrome? Moreover, it cannot depend on the input url.  
> > > > > > > 
> > > > > > > struct UIDNAWrapper {
> > > > > > >   UIDNAWrapper() {
> > > > > > >     UErrorCode err = U_ZERO_ERROR;
> > > > > > >     // TODO(jungshik): Change options as different parties
> (browsers,
> > > > > > >     // registrars, search engines) converge toward a consensus.
> > > > > > >     value = uidna_openUTS46(UIDNA_CHECK_BIDI, &err);
> > > > > > >     if (U_FAILURE(err))
> > > > > > >       value = NULL;
> > > > > > >   }
> > > > > > > 
> > > > > > >   UIDNA* value;
> > > > > > > };
> > > > > > 
> > > > > > I didn't observe this behavior in Chromium, but in modpagespeed. We
> > > > construct
> > > > > a
> > > > > > GoogleUrl with a user-provided string which causes a crash.
> > > > > 
> > > > > This is not just an issue with modpagespeed, but in any place that
> > GoogleUrl
> > > > is
> > > > > constructed from user-supplied input, I just haven't tried to identify
> it
> > in
> > > > any
> > > > > other places yet.
> > > > 
> > > > My question is how |uidna == null| can be input-dependent. (i.e how the
> > > failure
> > > > of |uidna_openUTS46| can depend on a user-input when it does not take
any
> > user
> > > > input).
> > > 
> > > I'm not really sure, spotted this bug a few months ago and made this patch
> to
> > > our local copy without doing /too/ much investigation.
> > > You can repro it by constructing a GURL with "http://127.0.0.\230:8080/"
as
> > the
> > > argument.
> > > 
> > > If you'd like I can do some more digging, but it is definitely
reproducible,
> > and
> > > any consumers of this who construct GURL(untrusted_user_input) will
probably
> > > experience the same crash.
> > 
> > Do you have a crash when you pass a perfectly normal url like
> > 'http://www.google.com'? I suspect you do.
> 
> Maybe not that one because it's all ASCII. How about 'http://청와대.kr' ?

The reason I'm asking this question is that I suspect that the ICU data is not
loaded when you use this in modpagespeed. Then, uidna_openUTS46 will fail
because there's no ICU data to get UTS46 rules from.

[jcrowell, 2015-04-09 18:38:34]

On 2015/04/09 18:26:24, Jungshik at google wrote:
> On 2015/04/01 20:09:40, Jungshik at google wrote:
> > On 2015/04/01 19:17:36, Jungshik at google wrote:
> > > On 2015/04/01 14:20:22, jcrowell wrote:
> > > > On 2015/04/01 08:00:09, Jungshik at google wrote:
> > > > > On 2015/03/31 15:28:45, jcrowell wrote:
> > > > > > On 2015/03/30 14:17:03, jcrowell wrote:
> > > > > > > On 2015/03/27 21:59:16, Jungshik at google wrote:
> > > > > > > >
> > > >
https://codereview.chromium.org/881073002/diff/20001/url/url_canon_icu.cc
> > > > > > > > File url/url_canon_icu.cc (right):
> > > > > > > > 
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/881073002/diff/20001/url/url_canon_icu.cc#new...
> > > > > > > > url/url_canon_icu.cc:168: }
> > > > > > > > This should not happen. If it happens, we have to track down the
> > root
> > > > > cause.
> > > > > > 
> > > > > > > > g_uidna is obtained here and I wonder why uidna_openUTS46 is
> > failing.
> > > Is
> > > > > it
> > > > > > > > failing in Chrome? Moreover, it cannot depend on the input url. 

> > > > > > > > 
> > > > > > > > struct UIDNAWrapper {
> > > > > > > >   UIDNAWrapper() {
> > > > > > > >     UErrorCode err = U_ZERO_ERROR;
> > > > > > > >     // TODO(jungshik): Change options as different parties
> > (browsers,
> > > > > > > >     // registrars, search engines) converge toward a consensus.
> > > > > > > >     value = uidna_openUTS46(UIDNA_CHECK_BIDI, &err);
> > > > > > > >     if (U_FAILURE(err))
> > > > > > > >       value = NULL;
> > > > > > > >   }
> > > > > > > > 
> > > > > > > >   UIDNA* value;
> > > > > > > > };
> > > > > > > 
> > > > > > > I didn't observe this behavior in Chromium, but in modpagespeed.
We
> > > > > construct
> > > > > > a
> > > > > > > GoogleUrl with a user-provided string which causes a crash.
> > > > > > 
> > > > > > This is not just an issue with modpagespeed, but in any place that
> > > GoogleUrl
> > > > > is
> > > > > > constructed from user-supplied input, I just haven't tried to
identify
> > it
> > > in
> > > > > any
> > > > > > other places yet.
> > > > > 
> > > > > My question is how |uidna == null| can be input-dependent. (i.e how
the
> > > > failure
> > > > > of |uidna_openUTS46| can depend on a user-input when it does not take
> any
> > > user
> > > > > input).
> > > > 
> > > > I'm not really sure, spotted this bug a few months ago and made this
patch
> > to
> > > > our local copy without doing /too/ much investigation.
> > > > You can repro it by constructing a GURL with "http://127.0.0.\230:8080/"
> as
> > > the
> > > > argument.
> > > > 
> > > > If you'd like I can do some more digging, but it is definitely
> reproducible,
> > > and
> > > > any consumers of this who construct GURL(untrusted_user_input) will
> probably
> > > > experience the same crash.
> > > 
> > > Do you have a crash when you pass a perfectly normal url like
> > > 'http://www.google.com'? I suspect you do.
> > 
> > Maybe not that one because it's all ASCII. How about 'http://청와대.kr' ?
> 
> The reason I'm asking this question is that I suspect that the ICU data is not
> loaded when you use this in modpagespeed. Then, uidna_openUTS46 will fail
> because there's no ICU data to get UTS46 rules from.

looking into this now, thanks.

[jungshik at Google, 2015-09-02 19:06:50]

On 2015/04/09 18:38:34, jcrowell wrote:
> On 2015/04/09 18:26:24, Jungshik at google wrote:
> > On 2015/04/01 20:09:40, Jungshik at google wrote:
> > > On 2015/04/01 19:17:36, Jungshik at google wrote:
> > > > On 2015/04/01 14:20:22, jcrowell wrote:
> > > > > On 2015/04/01 08:00:09, Jungshik at google wrote:
> > > > > > On 2015/03/31 15:28:45, jcrowell wrote:
> > > > > > > On 2015/03/30 14:17:03, jcrowell wrote:
> > > > > > > > On 2015/03/27 21:59:16, Jungshik at google wrote:
> > > > > > > > >
> > > > >
> https://codereview.chromium.org/881073002/diff/20001/url/url_canon_icu.cc
> > > > > > > > > File url/url_canon_icu.cc (right):
> > > > > > > > > 
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/881073002/diff/20001/url/url_canon_icu.cc#new...
> > > > > > > > > url/url_canon_icu.cc:168: }
> > > > > > > > > This should not happen. If it happens, we have to track down
the
> > > root
> > > > > > cause.
> > > > > > > 
> > > > > > > > > g_uidna is obtained here and I wonder why uidna_openUTS46 is
> > > failing.
> > > > Is
> > > > > > it
> > > > > > > > > failing in Chrome? Moreover, it cannot depend on the input
url. 
> 
> > > > > > > > > 
> > > > > > > > > struct UIDNAWrapper {
> > > > > > > > >   UIDNAWrapper() {
> > > > > > > > >     UErrorCode err = U_ZERO_ERROR;
> > > > > > > > >     // TODO(jungshik): Change options as different parties
> > > (browsers,
> > > > > > > > >     // registrars, search engines) converge toward a
consensus.
> > > > > > > > >     value = uidna_openUTS46(UIDNA_CHECK_BIDI, &err);
> > > > > > > > >     if (U_FAILURE(err))
> > > > > > > > >       value = NULL;
> > > > > > > > >   }
> > > > > > > > > 
> > > > > > > > >   UIDNA* value;
> > > > > > > > > };
> > > > > > > > 
> > > > > > > > I didn't observe this behavior in Chromium, but in modpagespeed.
> We
> > > > > > construct
> > > > > > > a
> > > > > > > > GoogleUrl with a user-provided string which causes a crash.
> > > > > > > 
> > > > > > > This is not just an issue with modpagespeed, but in any place that
> > > > GoogleUrl
> > > > > > is
> > > > > > > constructed from user-supplied input, I just haven't tried to
> identify
> > > it
> > > > in
> > > > > > any
> > > > > > > other places yet.
> > > > > > 
> > > > > > My question is how |uidna == null| can be input-dependent. (i.e how
> the
> > > > > failure
> > > > > > of |uidna_openUTS46| can depend on a user-input when it does not
take
> > any
> > > > user
> > > > > > input).
> > > > > 
> > > > > I'm not really sure, spotted this bug a few months ago and made this
> patch
> > > to
> > > > > our local copy without doing /too/ much investigation.
> > > > > You can repro it by constructing a GURL with
"http://127.0.0.\230:8080/"
> > as
> > > > the
> > > > > argument.
> > > > > 
> > > > > If you'd like I can do some more digging, but it is definitely
> > reproducible,
> > > > and
> > > > > any consumers of this who construct GURL(untrusted_user_input) will
> > probably
> > > > > experience the same crash.
> > > > 
> > > > Do you have a crash when you pass a perfectly normal url like
> > > > 'http://www.google.com'? I suspect you do.
> > > 
> > > Maybe not that one because it's all ASCII. How about 'http://청와대.kr' ?
> > 
> > The reason I'm asking this question is that I suspect that the ICU data is
not
> > loaded when you use this in modpagespeed. Then, uidna_openUTS46 will fail
> > because there's no ICU data to get UTS46 rules from.
> 
> looking into this now, thanks.

I took a look at mod_pagespeed and it has 'use_system_icu' set to 1. If the
version of ICU (as installed on OS such as Ubuntu) is outdated  (older than ICU
4.6 [1]), it'd not have UTS46 data and uidna_openUTS46 will fail. 

You can apply the url_canon_icu part of a CL at
https://codereview.chromium.org/1230093002/ and see what error you get from
uidna_openUTS46. 



[1] http://bugs.icu-project.org/trac/ticket/7144