[jitsi-dev] [ice4j] r361 committed - Fixes NPE when InetAddress is null in TransportAddress.getHostAddress(...


#1

Revision: 361
Author: paweldomas@gmail.com
Date: Wed Nov 13 20:43:04 2013 UTC
Log: Fixes NPE when InetAddress is null in
TransportAddress.getHostAddress().
http://code.google.com/p/ice4j/source/detail?r=361

Modified:
/trunk/src/org/ice4j/TransportAddress.java

=======================================
--- /trunk/src/org/ice4j/TransportAddress.java Mon Jun 24 21:06:31 2013 UTC
+++ /trunk/src/org/ice4j/TransportAddress.java Wed Nov 13 20:43:04 2013 UTC
@@ -155,7 +155,8 @@
     public String getHostAddress()
     {
         InetAddress addr = getAddress();
- String addressStr = addr.getHostAddress();
+ String addressStr
+ = addr != null ? addr.getHostAddress() : "null";

Are we sure about "null"? As far as my understading goes, we are
sending this over the network and not using it for display purposes
only. Wouldn't it be better to let the method caller to check for null
instead of "null"?

···

2013/11/13 <ice4j@googlecode.com>:

         if(addr instanceof Inet6Address)
             addressStr = NetworkUtils.stripScopeID(addressStr);


#2

Hi Lubomir,

Revision: 361
Author: paweldomas@gmail.com
Date: Wed Nov 13 20:43:04 2013 UTC
Log: Fixes NPE when InetAddress is null in
TransportAddress.getHostAddress().
http://code.google.com/p/ice4j/source/detail?r=361

Modified:
/trunk/src/org/ice4j/TransportAddress.java

=======================================
--- /trunk/src/org/ice4j/TransportAddress.java Mon Jun 24 21:06:31 2013 UTC
+++ /trunk/src/org/ice4j/TransportAddress.java Wed Nov 13 20:43:04 2013 UTC
@@ -155,7 +155,8 @@
     public String getHostAddress()
     {
         InetAddress addr = getAddress();
- String addressStr = addr.getHostAddress();
+ String addressStr
+ = addr != null ? addr.getHostAddress() : "null";

Are we sure about "null"? As far as my understading goes, we are
sending this over the network and not using it for display purposes
only. Wouldn't it be better to let the method caller to check for null
instead of "null"?

No, I'm not sure about that.
I have NPE on machine which does not support ipv6 when running this code:

test.IcePseudoTcp:72
// STUN
        if (USE_STUN)
        {
            StunCandidateHarvester stunHarv = new StunCandidateHarvester(
                new TransportAddress("sip-communicator.net",
                                     3478, Transport.UDP));
            StunCandidateHarvester stun6Harv = new StunCandidateHarvester(
                new TransportAddress("ipv6.sip-communicator.net",
                                     3478, Transport.UDP));

            agent.addCandidateHarvester(stunHarv);
            agent.addCandidateHarvester(stun6Harv);
        }

Without this fix code fails completely. With this fix fails only ipv6
harvester when sending binding request to null:3478, but the whole
code runs. What else can we put there instead of null ? I guess we
don't have any string for "invalid" host name ?

Regards,
Pawel

···

On Wed, Nov 13, 2013 at 10:16 PM, Lyubomir Marinov <lyubomir.marinov@jitsi.org> wrote:

2013/11/13 <ice4j@googlecode.com>:


#3

TransportAddress is relatively low level for that kind of modification.

Pawel, do you know what the reason is for that address to be null? It
sounds like it shouldn't have been returned by the harvester in the first
place.

--sent from my mobile

···

On 13 Nov 2013 22:36, "Paweł Domas" <pawel.domas@jitsi.org> wrote:

Hi Lubomir,

On Wed, Nov 13, 2013 at 10:16 PM, Lyubomir Marinov > <lyubomir.marinov@jitsi.org> wrote:
> 2013/11/13 <ice4j@googlecode.com>:
>> Revision: 361
>> Author: paweldomas@gmail.com
>> Date: Wed Nov 13 20:43:04 2013 UTC
>> Log: Fixes NPE when InetAddress is null in
>> TransportAddress.getHostAddress().
>> http://code.google.com/p/ice4j/source/detail?r=361
>>
>> Modified:
>> /trunk/src/org/ice4j/TransportAddress.java
>>
>> =======================================
>> --- /trunk/src/org/ice4j/TransportAddress.java Mon Jun 24 21:06:31
2013 UTC
>> +++ /trunk/src/org/ice4j/TransportAddress.java Wed Nov 13 20:43:04
2013 UTC
>> @@ -155,7 +155,8 @@
>> public String getHostAddress()
>> {
>> InetAddress addr = getAddress();
>> - String addressStr = addr.getHostAddress();
>> + String addressStr
>> + = addr != null ? addr.getHostAddress() : "null";
>
> Are we sure about "null"? As far as my understading goes, we are
> sending this over the network and not using it for display purposes
> only. Wouldn't it be better to let the method caller to check for null
> instead of "null"?

No, I'm not sure about that.
I have NPE on machine which does not support ipv6 when running this code:

test.IcePseudoTcp:72
// STUN
        if (USE_STUN)
        {
            StunCandidateHarvester stunHarv = new StunCandidateHarvester(
                new TransportAddress("sip-communicator.net",
                                     3478, Transport.UDP));
            StunCandidateHarvester stun6Harv = new StunCandidateHarvester(
                new TransportAddress("ipv6.sip-communicator.net",
                                     3478, Transport.UDP));

            agent.addCandidateHarvester(stunHarv);
            agent.addCandidateHarvester(stun6Harv);
        }

Without this fix code fails completely. With this fix fails only ipv6
harvester when sending binding request to null:3478, but the whole
code runs. What else can we put there instead of null ? I guess we
don't have any string for "invalid" host name ?

Regards,
Pawel

_______________________________________________
dev mailing list
dev@jitsi.org
Unsubscribe instructions and other list options:
http://lists.jitsi.org/mailman/listinfo/dev


#4

Hi Emil, Lubomir,

Docs for InetSocketAddress.getAddress() say:

"the InetAdress or <code>null</code> if it is unresolved."

So we have to handle null. I guess we should return null in this case
and check for it later.

When I make it this way it breaks one step further:

Exception in thread "main" java.lang.NullPointerException
at java.lang.StringBuilder.<init>(StringBuilder.java:109)
at org.ice4j.TransportAddress.toString(TransportAddress.java:139)
at java.lang.String.valueOf(String.java:2854)
at java.lang.StringBuilder.append(StringBuilder.java:128)
at org.ice4j.ice.harvest.StunCandidateHarvester.toString(StunCandidateHarvester.java:396)
at org.ice4j.ice.harvest.CandidateHarvesterSetElement.<init>(CandidateHarvesterSetElement.java:56)
at org.ice4j.ice.harvest.CandidateHarvesterSet.add(CandidateHarvesterSet.java:73)
at org.ice4j.ice.Agent.addCandidateHarvester(Agent.java:767)
at test.IcePseudoTcp.createAgent(IcePseudoTcp.java:83)
at test.IcePseudoTcp.main(IcePseudoTcp.java:291)

Then I try to fix it in org.ice4j.TransportAddress.toString to check
for null host address, but then I have to return null from toString
which also doesn't seem to be right. But the code works in this case -
it just disables this ipv6 stun harvester.

Any suggestions ?

Regards,
Pawel

···

On Wed, Nov 13, 2013 at 11:29 PM, Emil Ivov <emcho@jitsi.org> wrote:

TransportAddress is relatively low level for that kind of modification.

Pawel, do you know what the reason is for that address to be null? It sounds
like it shouldn't have been returned by the harvester in the first place.

--sent from my mobile

On 13 Nov 2013 22:36, "Paweł Domas" <pawel.domas@jitsi.org> wrote:

Hi Lubomir,

On Wed, Nov 13, 2013 at 10:16 PM, Lyubomir Marinov >> <lyubomir.marinov@jitsi.org> wrote:
> 2013/11/13 <ice4j@googlecode.com>:
>> Revision: 361
>> Author: paweldomas@gmail.com
>> Date: Wed Nov 13 20:43:04 2013 UTC
>> Log: Fixes NPE when InetAddress is null in
>> TransportAddress.getHostAddress().
>> http://code.google.com/p/ice4j/source/detail?r=361
>>
>> Modified:
>> /trunk/src/org/ice4j/TransportAddress.java
>>
>> =======================================
>> --- /trunk/src/org/ice4j/TransportAddress.java Mon Jun 24 21:06:31
>> 2013 UTC
>> +++ /trunk/src/org/ice4j/TransportAddress.java Wed Nov 13 20:43:04
>> 2013 UTC
>> @@ -155,7 +155,8 @@
>> public String getHostAddress()
>> {
>> InetAddress addr = getAddress();
>> - String addressStr = addr.getHostAddress();
>> + String addressStr
>> + = addr != null ? addr.getHostAddress() : "null";
>
> Are we sure about "null"? As far as my understading goes, we are
> sending this over the network and not using it for display purposes
> only. Wouldn't it be better to let the method caller to check for null
> instead of "null"?

No, I'm not sure about that.
I have NPE on machine which does not support ipv6 when running this code:

test.IcePseudoTcp:72
// STUN
        if (USE_STUN)
        {
            StunCandidateHarvester stunHarv = new StunCandidateHarvester(
                new TransportAddress("sip-communicator.net",
                                     3478, Transport.UDP));
            StunCandidateHarvester stun6Harv = new StunCandidateHarvester(
                new TransportAddress("ipv6.sip-communicator.net",
                                     3478, Transport.UDP));

            agent.addCandidateHarvester(stunHarv);
            agent.addCandidateHarvester(stun6Harv);
        }

Without this fix code fails completely. With this fix fails only ipv6
harvester when sending binding request to null:3478, but the whole
code runs. What else can we put there instead of null ? I guess we
don't have any string for "invalid" host name ?

Regards,
Pawel

_______________________________________________
dev mailing list
dev@jitsi.org
Unsubscribe instructions and other list options:
http://lists.jitsi.org/mailman/listinfo/dev

_______________________________________________
dev mailing list
dev@jitsi.org
Unsubscribe instructions and other list options:
http://lists.jitsi.org/mailman/listinfo/dev


#5

Then I try to fix it in org.ice4j.TransportAddress.toString to check
for null host address, but then I have to return null from toString
which also doesn't seem to be right. But the code works in this case -
it just disables this ipv6 stun harvester.

Any suggestions ?

The Javadoc of .toString() says:
     * If the address is unresolved then the
     * part before the colon will only contain the host name.

I don't know how TransportAddress is initialized in your null case, but in any way it should be possible to return something meaningful.

Regards,
Pawel

Ingo


#6

Hi Ingo,

···

On Thu, Nov 14, 2013 at 9:44 AM, Ingo Bauersachs <ingo@jitsi.org> wrote:

Then I try to fix it in org.ice4j.TransportAddress.toString to check
for null host address, but then I have to return null from toString
which also doesn't seem to be right. But the code works in this case -
it just disables this ipv6 stun harvester.

Any suggestions ?

The Javadoc of .toString() says:
     * If the address is unresolved then the
     * part before the colon will only contain the host name.

I don't know how TransportAddress is initialized in your null case, but in any way it should be possible to return something meaningful.

Thanks ! It makes sense. I'll provide a fix for it later today.

Regards,
Pawel


#7

Well that's the thing: TransportAddresses are initialized by either
Harvesters or signalling. In either case there's no resolving to do. I
am not sure how we get in the case where the address is null, which is
why I asked. My suspicion is that some harvester is operating weirdly
on android but if that happens we should detect the problem right
there and not return the address in the first place.

See what I mean? Pawel could you please have a look at that?

Emil

···

On Thu, Nov 14, 2013 at 9:44 AM, Ingo Bauersachs <ingo@jitsi.org> wrote:

Then I try to fix it in org.ice4j.TransportAddress.toString to check
for null host address, but then I have to return null from toString
which also doesn't seem to be right. But the code works in this case -
it just disables this ipv6 stun harvester.

Any suggestions ?

The Javadoc of .toString() says:
     * If the address is unresolved then the
     * part before the colon will only contain the host name.

I don't know how TransportAddress is initialized in your null case, but in any way it should be possible to return something meaningful.