search

MiniDNS / minidns

DNS library for Android and Java SE
Other
215 stars 61 forks source link

IllegalArgumentException from minidns-hla 0.3.4 #104

Open quite opened 4 years ago

quite commented 4 years ago

I've seen this in my app Mumla. Got a stack trace from a user, sent to me by gôôg play. I'm using 0.3.4 from jcenter which apparently is not tagged in this repo (!). Have #70 been resurrected?

java.lang.IllegalArgumentException: 
  _at java.net.IDN.toASCII (IDN.java:115)
  _at java.net.IDN.toASCII (IDN.java:138)
  at org.minidns.idna.DefaultIdnaTransformator.toASCII (DefaultIdnaTransformator.java:28)
  at org.minidns.idna.MiniDnsIdna.toASCII (MiniDnsIdna.java:18)
  at org.minidns.dnsname.DnsName.<init> (DnsName.java:126)
  at org.minidns.dnsname.DnsName.from (DnsName.java:331)
  at org.minidns.hla.ResolverApi.resolveSrv (ResolverApi.java:140)
  at se.lublin.humla.model.Server$2.run (Server.java:200)
  _at java.lang.Thread.run (Thread.java:929)
  _at android.icu.impl.IDNA2003.convertToASCII
  _at android.icu.impl.IDNA2003.convertIDNToASCII (IDNA2003.java:277)
  _at android.icu.text.IDNA.convertIDNToASCII (IDNA.java:654)
  _at java.net.IDN.toASCII (IDN.java:108)

(lines marked with _ are lighter gray in the report)

This is my code:

final String lookup = "_mumble._tcp." + srvHost.get();
SrvResolverResult res = ResolverApi.INSTANCE.resolveSrv(lookup);
if (!res.wasSuccessful()) {

Unfortunately I cannot see what srvHost it was, from user input...

Flowdalic commented 4 years ago

Thanks for your report.

I'm using 0.3.4 from jcenter which apparently is not tagged in this repo (!).

Thanks for pointing this out. I have just pushed the 0.3.4 tag.

Have #70 been resurrected?

I am not sure about that. #70 was caused by Android bug https://issuetracker.google.com/issues/113070416 for which have https://github.com/MiniDNS/minidns/blob/6d8ec0494cd15a8aa01ff729da1b6ecb0bcd2c04/minidns-core/src/main/java/org/minidns/idna/DefaultIdnaTransformator.java#L21-L26 in place.

The stacktrace has some noticeable characteristics: First, is the exception's message really the empty string? And why does it look like convertToASCII is calling Thread.run()?

If we would know the used Android version, we could look into AOSP and see what is triggering the IAE at IDN.java:115.

Thanks for using MiniDNS btw. Especially for a mumble related project, which I currently use nearly daily. Please feel hereby encouraged to try out the latest minidns 0.4 alpha: 0.4.0-alpha6. I has many Android related improvements, and even though it is an alpha version, it runs pretty stable. And the MiniDNS project needs testers who can provide feedback. I think you could use it in production, but an other option would be to integrate the new MiniDNS version in your staging/development branch of your app.

BTW, I noticed that you do not include minidns-android21 in your project. You really should do so, and call AndroidUsingLinkProperties setup(Context context) before the first DNS lookup.

quite commented 4 years ago

The stacktrace has some noticeable characteristics: First, is the exception's message really the empty string?

Where is a message supposed to be? I pasted what I got.

And why does it look like convertToASCII is calling Thread.run()?

Yes, it does look weird. I see things like that now and then, perhaps it is precisely related to threading, and the unwinding of the stack around that. The Thread.run line in the stack trace, and those below are in a lighter gray color in the report (as are the first two lines), I don't know if that means something important. Couldn't find anything with a quick search.

If we would know the used Android version, we could look into AOSP and see what is triggering the IAE at IDN.java:115.

Ah yes, it's Android 10.

Thanks for using MiniDNS btw. Especially for a mumble related project, which I currently use nearly daily.

Nice! If you need an up-to-date Android app you know what it's called now :)

Please feel hereby encouraged to try out the latest minidns 0.4 alpha: 0.4.0-alpha6. I has many Android related improvements, and even though it is an alpha version, it runs pretty stable. And the MiniDNS project needs testers who can provide feedback. I think you could use it in production, but an other option would be to integrate the new MiniDNS version in your staging/development branch of your app.

Mmm, I might try that when I get time. I was cautious about the "alpha" obviously...

BTW, I noticed that you do not include minidns-android21 in your project. You really should do so, and call AndroidUsingLinkProperties setup(Context context) before the first DNS lookup.

Thanks! I'll look at that.

Flowdalic commented 4 years ago

Where is a message supposed to be? I pasted what I got.

It is printed right after the exception class. I.e., java.lang.IllegalArgumentException: <here would be the exception message>

Since IDN.java:115 is https://android.googlesource.com/platform/libcore/+/master/ojluni/src/main/java/java/net/IDN.java#115 it appears we are missing the complete exception message. Which is sad, since it contains the crucial information.

We may run into the same issue okhttp has experienced: https://github.com/square/okhttp/issues/5840

Android 10's IDN.toASCII() does stricter enforcing, which itself is appears correct. MiniDNS eventually should not call toASCII() on Non-LDH DNS labels. Which, on the other hand, does not appear to be trivial, since toASCII() is currently called by MiniDNS with the full DNS name, and not with individual labels. However, I think to ASCII() should operate on individual labels, and not on the full DNS name.

I see if I can push a fix for this.

Mmm, I might try that when I get time. I was cautious about the "alpha" obviously...

I know, but if everyone waits until the official release, then the only difference between an alpha release and a release is just the string. ;)

quite commented 4 years ago

it appears we are missing the complete exception message. Which is sad, since it contains the crucial information.

Goog is stripping it and it is no longer possible to get hold of (it used to be available in some downloadable reports). Perhaps devs stuffed integrity problematic things into it.... or goog is just pushing towards use of Firebase Crashlytics, which seems to have more detailed crash reports.

We may run into the same issue okhttp has experienced: square/okhttp#5840

I skimmed very quickly through that... But underscores does work, at least in the _mumble._tcp part.

MiniDNS eventually should not call to on Non-LDH DNS labels

Should that be "should call toASCII() only on non-LDH DNS labels"? (LDH being letters, digits, hyphen, right? but not underscore).

Could the issue be a label with international unicode letters plus underscore? Do they exist? Well, they can surely be input... Just thinking aloud.

Flowdalic commented 4 years ago

Could the issue be a label with international unicode letters plus underscore? Do they exist?

That is very well possible. Underscore(s) (U+00EF) are not allowed in U-Labels, see RFC 5892 § B.1: 003A..0060 ; DISALLOWED # COLON..GRAVE ACCENT but I can not rule out that you find those in the wild.

Fun fact: They are, however, allowed in DNS labels. Any byte is allowed in DNS labels. They are not allowed in LDH labels, obviously. But all that is not relevant here.

Eventually, I am afraid, there is not much I can do right now, as the causing String is not know.

quite commented 4 years ago

but I can not rule out that you find those in the wild.

If I add a server to my add with foo.ö_n.com in the address field, it crashes. And, it is now actually bricked! on Android 10 that is. Because it tries to ping all the servers in the list upon startup. (i think ö_n.example.com was fine).

...or so I thought. But now I cant reproduce it!

Fun fact: They are, however, allowed in DNS labels. Any byte is allowed in DNS labels. But that is not relevant here.

I know! Left to the implementors to do as they please, and boy, did they ever :)

Flowdalic commented 4 years ago

Ultimately this means that you want to wrap the call in a try/catch block and catch the IllegalArgumentException.

quite commented 4 years ago

Yup, I did that just now.

I also found this in the syslog! It seems to error on the empty label that I typed by mistake! Not intl unicode.

FATAL EXCEPTION: Thread-32
Process: se.lublin.mumla, PID: 28768
java.lang.IllegalArgumentException: Invalid input to toASCII: _mumble._tcp..k_ö.com
        at java.net.IDN.toASCII(IDN.java:115)
        at java.net.IDN.toASCII(IDN.java:138)
        at org.minidns.idna.DefaultIdnaTransformator.toASCII(DefaultIdnaTransformator.java:28)
        at org.minidns.idna.MiniDnsIdna.toASCII(MiniDnsIdna.java:18)
        at org.minidns.dnsname.DnsName.<init>(DnsName.java:126)
        at org.minidns.dnsname.DnsName.from(DnsName.java:331)
        at org.minidns.hla.ResolverApi.resolveSrv(ResolverApi.java:140)
        at se.lublin.humla.model.Server$2.run(Server.java:200)
        at java.lang.Thread.run(Thread.java:919)
Caused by: Found zero length lable after NamePrep.. line:  0. preContext:  . postContext:

        at android.icu.impl.IDNA2003.convertToASCII(IDNA2003.java:187)
        at android.icu.impl.IDNA2003.convertIDNToASCII(IDNA2003.java:277)
        at android.icu.text.IDNA.convertIDNToASCII(IDNA.java:654)
        at java.net.IDN.toASCII(IDN.java:108)
        ... 8 more
quite commented 3 years ago

Wondering if this is an issue in 0.4.0. Guess I'd just have to try :>