[jitsi-dev] ice4j patches for Turnserver


#1

Hello we will be using this thread to send patches for the ice4j as
required by TurnServer.

Regards,
*Aakash Garg*


#2

Hi,

Apart from the previous patches sent (now committed) please find attached
the following patches:

I have committed on git for TurnServer. Please find attached patches for
the ice4j for the classification,relaying.

*stunStackAddSendFun.patch* - adds Constructor overload, sendChannelData
method, sendUdpMessage method, and other functions for TurnStack and
relaying in StunStack.

*NetAccessmanagerOverloadFun.patch - *adds constructor overload,
sendMessage overload for ChannelData nad bytes[] and other fields with
getters.

*NewClassesForClassification.patch* - Adds new classes new messageEvent
with their listener interfaces.

*MessageProcessorClassificationLogic.patch* - Adds logic for classifying
incoming messages between Stun message, ChannelData and UDP message from
peer.

*MessageFactoryAddCreateDataIndication.patch* - Adds createDataIndication
method.

*ReservationTokenGeneration.patch* - Adds method to generate
Reservation-Token and toString method.

*ChannelDataValidFun.patch - *add function to check if message is
ChannelData Message in ChannelData class.

Regards,
Aakash Garg.

ChannelDataValidFun.patch (965 Bytes)

MessageProcessorClassificationLogic.patch (8.14 KB)

NewClassesForClassification.patch (9.04 KB)

stunStackAddSendFun.patch (6.53 KB)

NetAccessmanagerOverloadFun.patch (7.66 KB)

PeerUdpMessageEventHandler.patch (1.19 KB)

ChannelDataEventHandler.patch (1.18 KB)

MessageFactoryAddCreateDataIndication.patch (1.71 KB)

ReservationTokenGeneration.patch (5.35 KB)

···

On Sat, Jun 14, 2014 at 3:50 PM, Aakash Garg <aakashgargnsit@gmail.com> wrote:

Hello we will be using this thread to send patches for the ice4j as
required by TurnServer.

Regards,
*Aakash Garg*

--
*Aakash Garg*
*Netaji Subhas Institute of Technology (NSIT), Dwarka,*
*New Delhi*


#3

Hi Aakash,

Thanks for the patches !

ChannelDataValidFun.patch - add function to check if message is ChannelData
Message in ChannelData class.

Like we've discussed earlier the comparision below[1] will not check
if this byte has first two bits set to 0b01. Comparision [2] should be
used instead.

[1]:
if (binMessage[0] >> 2 == 0x1)

[2]:
if (binMessage[0] >> 6 == 0x1)

stunStackAddSendFun.patch - adds Constructor overload, sendChannelData
method, sendUdpMessage method, and other functions for TurnStack and
relaying in StunStack.

Do we need to add them to StunStack class if those methods are used
only by TurnStack ? Or am I missing something ?

NewClassesForClassification.patch - Adds new classes new messageEvent with
their listener interfaces.

Javadoc text exceeds 80 column limit. Not all methods are documented.
Not all method argument are described. In PeerUdpMessageEvent often
channel data term is used instead of peer udp message.

MessageProcessorClassificationLogic.patch - Adds logic for classifying
incoming messages between Stun message, ChannelData and UDP message from
peer.

MessageProcessor:186 swallows the exception. We should log it at least
and/or use errorHandler.

MessageFactoryAddCreateDataIndication.patch - Adds createDataIndication
method.

Commited.

ReservationTokenGeneration.patch - Adds method to generate Reservation-Token
and toString method.

What's the purpose of storing hashCode in class field if it's never
accessed after that ? The field is private.

Regards,
Pawel

···

On Sat, Jun 14, 2014 at 12:30 PM, Aakash Garg <aakashgargnsit@gmail.com> wrote:


#4

Hi Pawel,

Hi Aakash,

Thanks for the patches !

> ChannelDataValidFun.patch - add function to check if message is
ChannelData
> Message in ChannelData class.

Like we've discussed earlier the comparision below[1] will not check
if this byte has first two bits set to 0b01. Comparision [2] should be
used instead.

[1]:
if (binMessage[0] >> 2 == 0x1)

[2]:
if (binMessage[0] >> 6 == 0x1)

Mistakenly attached the old patch. Attached the new patch now.

> stunStackAddSendFun.patch - adds Constructor overload, sendChannelData
> method, sendUdpMessage method, and other functions for TurnStack and
> relaying in StunStack.

Do we need to add them to StunStack class if those methods are used
only by TurnStack ? Or am I missing something ?

Since we are adding UDP event handler and ChannelData handler they may

require to send ChannelData and UDP messages for other protocols not
specifically TURN.
Hence added here in StunStack. Should I change it if it is inconvenient?

Also added the createCorrespondingErrorResponse as previously discussed to
avoid problems as discussed.

NewClassesForClassification.patch - Adds new classes new messageEvent with
> their listener interfaces.

Javadoc text exceeds 80 column limit. Not all methods are documented.
Not all method argument are described. In PeerUdpMessageEvent often
channel data term is used instead of peer udp message.

Corrected.

MessageProcessorClassificationLogic.patch - Adds logic for classifying
> incoming messages between Stun message, ChannelData and UDP message from
> peer.

MessageProcessor:186 swallows the exception. We should log it at least
and/or use errorHandler.

Added logger message. This exception is expected since there can be UDP

messages which has first 2 bits as 0x01.

MessageFactoryAddCreateDataIndication.patch - Adds createDataIndication
> method.

Commited.

> ReservationTokenGeneration.patch - Adds method to generate
Reservation-Token
> and toString method.
>

What's the purpose of storing hashCode in class field if it's never
accessed after that ? The field is private.

Added the hashcode function. this variable is calculated during generation
of new token.

Please provide suggestions.

Regards,
Pawel

Regards,
Aakash Garg

ChannelDataValidFun.patch (859 Bytes)

NewClassesForClassification.patch (9.63 KB)

ReservationTokenGeneration.patch (5.51 KB)

stunStackAddSendFun2.patch (10 KB)

···

On Sun, Jun 15, 2014 at 6:25 PM, Paweł Domas <pawel.domas@jitsi.org> wrote:

On Sat, Jun 14, 2014 at 12:30 PM, Aakash Garg <aakashgargnsit@gmail.com> > wrote:


#5

Hi Aakash,

Hi Pawel,

> ChannelDataValidFun.patch - add function to check if message is
> ChannelData
> Message in ChannelData class.

Like we've discussed earlier the comparision below[1] will not check
if this byte has first two bits set to 0b01. Comparision [2] should be
used instead.

[1]:
if (binMessage[0] >> 2 == 0x1)

[2]:
if (binMessage[0] >> 6 == 0x1)

Mistakenly attached the old patch. Attached the new patch now.

Thanks ! The patch is commited now.

> stunStackAddSendFun.patch - adds Constructor overload, sendChannelData
> method, sendUdpMessage method, and other functions for TurnStack and
> relaying in StunStack.

Do we need to add them to StunStack class if those methods are used
only by TurnStack ? Or am I missing something ?

Since we are adding UDP event handler and ChannelData handler they may
require to send ChannelData and UDP messages for other protocols not
specifically TURN.
Hence added here in StunStack. Should I change it if it is inconvenient?

Ok it's fine. Thanks for explaining this.

> MessageProcessorClassificationLogic.patch - Adds logic for classifying
> incoming messages between Stun message, ChannelData and UDP message from
> peer.

MessageProcessor:186 swallows the exception. We should log it at least
and/or use errorHandler.

Added logger message. This exception is expected since there can be UDP
messages which has first 2 bits as 0x01.

Yes, it is expected, but it might also swallow exceptions that are
unexpected, because Exception class is caught there. Maybe we should
narrow down Exception class to StunException. Then throw it when the
stack detects error during parsing of the attributes or when they make
no sense. Then we could silently discard StunException and log the
others.

Btw. you've forgot to include new version of this patch :wink:

Regards,
Pawel

···

On Sun, Jun 15, 2014 at 5:06 PM, Aakash Garg <aakashgargnsit@gmail.com> wrote:

On Sun, Jun 15, 2014 at 6:25 PM, Paweł Domas <pawel.domas@jitsi.org> wrote:

On Sat, Jun 14, 2014 at 12:30 PM, Aakash Garg <aakashgargnsit@gmail.com> >> wrote:


#6

Hi Pawel,

Hi Aakash,

> Hi Pawel,
>
>> > ChannelDataValidFun.patch - add function to check if message is
>> > ChannelData
>> > Message in ChannelData class.
>>
>> Like we've discussed earlier the comparision below[1] will not check
>> if this byte has first two bits set to 0b01. Comparision [2] should be
>> used instead.
>>
>> [1]:
>> if (binMessage[0] >> 2 == 0x1)
>>
>> [2]:
>> if (binMessage[0] >> 6 == 0x1)
>>
> Mistakenly attached the old patch. Attached the new patch now.

Thanks ! The patch is commited now.

>>
>> > stunStackAddSendFun.patch - adds Constructor overload, sendChannelData
>> > method, sendUdpMessage method, and other functions for TurnStack and
>> > relaying in StunStack.
>>
>> Do we need to add them to StunStack class if those methods are used
>> only by TurnStack ? Or am I missing something ?
>>
> Since we are adding UDP event handler and ChannelData handler they may
> require to send ChannelData and UDP messages for other protocols not
> specifically TURN.
> Hence added here in StunStack. Should I change it if it is inconvenient?

Ok it's fine. Thanks for explaining this.

>> > MessageProcessorClassificationLogic.patch - Adds logic for classifying
>> > incoming messages between Stun message, ChannelData and UDP message
from
>> > peer.
>>
>> MessageProcessor:186 swallows the exception. We should log it at least
>> and/or use errorHandler.
>>
> Added logger message. This exception is expected since there can be UDP
> messages which has first 2 bits as 0x01.

Yes, it is expected, but it might also swallow exceptions that are
unexpected, because Exception class is caught there. Maybe we should
narrow down Exception class to StunException. Then throw it when the
stack detects error during parsing of the attributes or when they make
no sense. Then we could silently discard StunException and log the
others.

Btw. you've forgot to include new version of this patch :wink:

Sorry for silly mistake. Please find attached new patch with the changes

as mentioned above :slight_smile:
Please provide suggestions if any.

Regards,
Pawel

Regards,
Aakash Garg

···

On Mon, Jun 16, 2014 at 12:54 AM, Paweł Domas <pawel.domas@jitsi.org> wrote:

On Sun, Jun 15, 2014 at 5:06 PM, Aakash Garg <aakashgargnsit@gmail.com> > wrote:
> On Sun, Jun 15, 2014 at 6:25 PM, Paweł Domas <pawel.domas@jitsi.org> > wrote:
>> On Sat, Jun 14, 2014 at 12:30 PM, Aakash Garg <aakashgargnsit@gmail.com > > > >> wrote:


#7

Sorry I again forgot the patch :slight_smile:

MessageProcessorClassificationLogic.patch (8.27 KB)

···

On Mon, Jun 16, 2014 at 2:34 AM, Aakash Garg <aakashgargnsit@gmail.com> wrote:

Hi Pawel,

On Mon, Jun 16, 2014 at 12:54 AM, Paweł Domas <pawel.domas@jitsi.org> > wrote:

Hi Aakash,

On Sun, Jun 15, 2014 at 5:06 PM, Aakash Garg <aakashgargnsit@gmail.com> >> wrote:
> Hi Pawel,
>
> On Sun, Jun 15, 2014 at 6:25 PM, Paweł Domas <pawel.domas@jitsi.org> >> wrote:
>> On Sat, Jun 14, 2014 at 12:30 PM, Aakash Garg < >> aakashgargnsit@gmail.com> >> >> wrote:
>> > ChannelDataValidFun.patch - add function to check if message is
>> > ChannelData
>> > Message in ChannelData class.
>>
>> Like we've discussed earlier the comparision below[1] will not check
>> if this byte has first two bits set to 0b01. Comparision [2] should be
>> used instead.
>>
>> [1]:
>> if (binMessage[0] >> 2 == 0x1)
>>
>> [2]:
>> if (binMessage[0] >> 6 == 0x1)
>>
> Mistakenly attached the old patch. Attached the new patch now.

Thanks ! The patch is commited now.

>>
>> > stunStackAddSendFun.patch - adds Constructor overload,
sendChannelData
>> > method, sendUdpMessage method, and other functions for TurnStack and
>> > relaying in StunStack.
>>
>> Do we need to add them to StunStack class if those methods are used
>> only by TurnStack ? Or am I missing something ?
>>
> Since we are adding UDP event handler and ChannelData handler they may
> require to send ChannelData and UDP messages for other protocols not
> specifically TURN.
> Hence added here in StunStack. Should I change it if it is inconvenient?

Ok it's fine. Thanks for explaining this.

>> > MessageProcessorClassificationLogic.patch - Adds logic for
classifying
>> > incoming messages between Stun message, ChannelData and UDP message
from
>> > peer.
>>
>> MessageProcessor:186 swallows the exception. We should log it at least
>> and/or use errorHandler.
>>
> Added logger message. This exception is expected since there can be UDP
> messages which has first 2 bits as 0x01.

Yes, it is expected, but it might also swallow exceptions that are
unexpected, because Exception class is caught there. Maybe we should
narrow down Exception class to StunException. Then throw it when the
stack detects error during parsing of the attributes or when they make
no sense. Then we could silently discard StunException and log the
others.

Btw. you've forgot to include new version of this patch :wink:

Sorry for silly mistake. Please find attached new patch with the changes

as mentioned above :slight_smile:
Please provide suggestions if any.

Regards,
Pawel

Regards,
Aakash Garg

--
*Aakash Garg*
*Netaji Subhas Institute of Technology (NSIT), Dwarka,*
*New Delhi*


#8

Hi Aakash,

Sorry I again forgot the patch :slight_smile:

I have few comments regards the patch:

1. I'm not sure if peer UDP messages classification is correct. These
are messages that go from peer to TURN client through the TURN server.
In normal exchange there will be large amount of those messages.
Current implementation recognizes them after StunException is thrown
from STUN message processing. This look like "exception oriented
programming" which has serious impact on performance. We need to think
on something more sophisticated. We should be able to check if TURN
server has currently allocation for remote address as well as other
conditions defined in [1]. So peer udp message handler should return
true if it has accepted the message, otherwise it will be passed for
other processors.

2. StunException that comes from ChannelData message processing should
be passed to the errorHandler like it's done during STUN messages
handling. Error handler by default logs them on higher log level.

[1]: http://tools.ietf.org/html/rfc5766#section-10.3

Regards,
Pawel

···

On Sun, Jun 15, 2014 at 11:06 PM, Aakash Garg <aakashgargnsit@gmail.com> wrote:


#9

Found a bug in decoding of LifetimeAttribute.

Please find attached fix of the bug mentioned.

Regards,
Aakash Garg.

LifetimeBug.patch (2.17 KB)

···

On Thu, Jun 19, 2014 at 5:19 PM, Paweł Domas <pawel.domas@jitsi.org> wrote:

Hi Aakash,

On Sun, Jun 15, 2014 at 11:06 PM, Aakash Garg <aakashgargnsit@gmail.com> > wrote:
> Sorry I again forgot the patch :slight_smile:

I have few comments regards the patch:

1. I'm not sure if peer UDP messages classification is correct. These
are messages that go from peer to TURN client through the TURN server.
In normal exchange there will be large amount of those messages.
Current implementation recognizes them after StunException is thrown
from STUN message processing. This look like "exception oriented
programming" which has serious impact on performance. We need to think
on something more sophisticated. We should be able to check if TURN
server has currently allocation for remote address as well as other
conditions defined in [1]. So peer udp message handler should return
true if it has accepted the message, otherwise it will be passed for
other processors.

2. StunException that comes from ChannelData message processing should
be passed to the errorHandler like it's done during STUN messages
handling. Error handler by default logs them on higher log level.

[1]: http://tools.ietf.org/html/rfc5766#section-10.3

Regards,
Pawel

--
*Aakash Garg*
*Netaji Subhas Institute of Technology (NSIT), Dwarka,*
*New Delhi*


#10

Hi,

Please find attached patches for the Turn Server for the ice4j.

   - *Adds_LongTermCredential_HashCodeAndEquals_Methods.patch* - Adds
   equals and hashCode function to LongTermCredentialMechanism.
   - *AddsStunStackSetter.patch *- Adds setStunStacksetter method in Agent
   class.
   - *BlockingRequestSender_LocalAddressgetter.patch* - Adds getter for
   localAddress in BlockingRequestSender
   - *ChangesConstructorAccessLevelICEpackage.patch* -Changes the access
   level of constructors of IceMediaStream & Component class.
   - *LifeTimeAttributeBugFix.patch *- ug fix in LifetimeAttribute decoding
   logic.
   - *MessageClassificationLogic.patch* - logic to classify the incoming
   messages as StunMessage/ChannelData message, data message.
   - *MessageFactoryNewMethod.patch* - Adds createConnectRequest() and
   updates the createConnnectionAttemptIndication to MessageFactory class.
   - *multipleTCPSocketsSupport.patch *- Adds fix for supporting multiple
   TCP sockets with the same source address.
   - *NewConnectionIdAttributeFunction.patch* -
   Adds createConnectionIdAttribute() in AttributeFactory class.
   - *Nonce.patch* - class for Nonce
   - *StackPropertiesnewFields.patch* - Adds new fields in StackProperties.

Please provide suggestions if any.

Regards,
Aakash Garg

Adds_LongTermCredential_HashCodeAndEquals_Methods.patch (1.1 KB)

AddsStunStackSetter.patch (874 Bytes)

BlockingRequestSender_LocalAddressgetter.patch (528 Bytes)

ChangesConstructorAccessLevelICEpackage.patch (1.52 KB)

LifeTimeAttributeBugFix.patch (2.1 KB)

MessageClassificationLogic.patch (8.12 KB)

MessageFactoryNewMethod.patch (3.89 KB)

multipleTCPSocketsSupport.patch (16.9 KB)

NewConnectionIdAttributeFunction.patch (758 Bytes)

Nonce.patch (6.55 KB)

StackPropertiesnewFields.patch (798 Bytes)

···

On Tue, Jul 1, 2014 at 6:51 PM, Aakash Garg <aakashgargnsit@gmail.com> wrote:

Found a bug in decoding of LifetimeAttribute.

Please find attached fix of the bug mentioned.

Regards,
Aakash Garg.

On Thu, Jun 19, 2014 at 5:19 PM, Paweł Domas <pawel.domas@jitsi.org> > wrote:

Hi Aakash,

On Sun, Jun 15, 2014 at 11:06 PM, Aakash Garg <aakashgargnsit@gmail.com> >> wrote:
> Sorry I again forgot the patch :slight_smile:

I have few comments regards the patch:

1. I'm not sure if peer UDP messages classification is correct. These
are messages that go from peer to TURN client through the TURN server.
In normal exchange there will be large amount of those messages.
Current implementation recognizes them after StunException is thrown
from STUN message processing. This look like "exception oriented
programming" which has serious impact on performance. We need to think
on something more sophisticated. We should be able to check if TURN
server has currently allocation for remote address as well as other
conditions defined in [1]. So peer udp message handler should return
true if it has accepted the message, otherwise it will be passed for
other processors.

2. StunException that comes from ChannelData message processing should
be passed to the errorHandler like it's done during STUN messages
handling. Error handler by default logs them on higher log level.

[1]: http://tools.ietf.org/html/rfc5766#section-10.3

Regards,
Pawel

--
*Aakash Garg*
*Netaji Subhas Institute of Technology (NSIT), Dwarka,*
*New Delhi*

--
*Aakash Garg*
*Netaji Subhas Institute of Technology (NSIT), Dwarka,*
*New Delhi*