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

Ignore invalid field length instead of crashing #159

Merged
merged 3 commits into from Apr 12, 2018

Conversation

Projects
None yet
3 participants
@Tolriq
Contributor

Tolriq commented Apr 12, 2018

As per title

@ViToni

This comment has been minimized.

Contributor

ViToni commented Apr 12, 2018

I would like to see the use case for this PR.

IMO this is only an issue when the user of jmdns tries to use invalid / too large text properties

@Tolriq

This comment has been minimized.

Contributor

Tolriq commented Apr 12, 2018

@ViToni as shown previously

Fatal Exception: java.lang.RuntimeException: Unexpected exception: java.io.IOException: Cannot have individual values larger that 255 chars. Offending value: TIMv�23�1�168�.92�in-�2�23��23�1�168�192�in�23�1�168�192�in-�2�2.��23�1�168�192�in�_googlecast�_tcp�local�x'.TIMv�23�1�168�.92�in-�2�23��23�1�168�192�in�23�1�168�192�in-�2�2.��23�1�168�192�in�_googlecast�_tcp�local�x'.TIMv�TIMvision.9141�_andr�T�T�TIMvision 9141�_a.T�T�TIM�TIMvis.9141��TIMvision 9141�_andr�T�T�T.9141�_a�T�T�TIM�TIMvision 9141��._tcp.local.
       at javax.jmdns.impl.ServiceInfoImpl.(ServiceInfoImpl.java:182)
       at javax.jmdns.impl.DNSRecord$Pointer.getServiceInfo(DNSRecord.java:546)
       at javax.jmdns.impl.DNSRecord$Pointer.getServiceEvent(DNSRecord.java:555)
       at javax.jmdns.impl.JmDNSImpl.updateRecord(JmDNSImpl.java:1272)
       at javax.jmdns.impl.JmDNSImpl.handleRecord(JmDNSImpl.java:1424)
       at javax.jmdns.impl.JmDNSImpl.handleQuery(JmDNSImpl.java:1536)
       at javax.jmdns.impl.SocketListener.run(SocketListener.java:59)

Some bad devices does trigger that even if we don't look for them.

The second part was modified after being asked by @kaikreuzer

@ViToni

This comment has been minimized.

Contributor

ViToni commented Apr 12, 2018

@Tolriq I am wondering if this might be more a bug in javax.jmdns.impl.DNSRecord.Pointer.getServiceInfo(boolean) or better in javax.jmdns.impl.DNSEntry.getQualifiedNameMap()

Could you please create an issue and attach a Wireshark dump of the offending request?

@Tolriq

This comment has been minimized.

Contributor

Tolriq commented Apr 12, 2018

I can't those are rare crash get on my crash reporting tool. No user ever contacted me about those so do not have more details than that.

@kaikreuzer

This comment has been minimized.

Member

kaikreuzer commented Apr 12, 2018

@ViToni I think it is ok to merge this PR - we will still have warnings in the log and if there is a bug in DNSRecord or DNSEntry, this can be fixed in a separate PR, once it can be figured out what it is.

@Tolriq I have added a commit, which fixes the null check in the logging statement.
I'll wait for Travis, but then it should be good to merge.
For future contributions, please make sure to sign your commits as stated in https://github.com/jmdns/jmdns/blob/master/CONTRIBUTING.md#sign-your-work. Many thanks!

@kaikreuzer kaikreuzer merged commit dcedd32 into jmdns:master Apr 12, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@kaikreuzer kaikreuzer added this to the 3.5.4 milestone Apr 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment